Remove ViewMsg_SetZoomLevel

Remove ViewMsg_SetZoomLevel

In an effort to simplify zoom, this CL removes ViewMsg_SetZoomLevel and plumbs zoom on WebContentsImpl directly through to HostZoomMap.

At present calls to WebContents::SetZoomLevel() invoke up to three cross-process IPCs, and introduce a synchronization requirement for any caller that needs to block until the results of the operation are visible via WebContents::GetZoomLevel().

This CL simplifies this down to one IPC and removal of the
synchronization requirement. It adds a new IPC message to signal changes in a view's ability to be zoomed, and removes an ViewMsg_SetZoomLevel that was used only by SetZoomLevel().

BUG=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273854

[wjmaclean, 2014-05-15 20:51:29]

Fady, can you take an initial look to see if this looks sane? It's incomplete,
in that I have not accounted for the HidePopups() behaviour that used to be part
of this pathway ... I'll add that into the next patch.

[Fady Samuel, 2014-05-15 23:26:53]

Let's chat about this tomorrow.

https://codereview.chromium.org/287093002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/287093002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host_impl.cc:1624:
GetProcess()->GetID(), GetRoutingID(), zoom_level);
The flow of all this code can get pretty confusing pretty fast. I agree this
needs to get fixed. I'd like to see diagrams tomorrow.

https://codereview.chromium.org/287093002/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/287093002/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:2282: BrowserPluginEmbedder*
embedder = GetBrowserPluginEmbedder();
If this is the main blocker to major code cleanup then let's not worry about
this. Let's get rid of this and figure out an alternative solution elsewhere. I
want to get rid of WebContentsImpl::SetZoomLevel completely.

https://codereview.chromium.org/287093002/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/287093002/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:2172: if
(GetRenderManager()->current_host()->uses_temporary_zoom_settings()) {
GetRenderViewHost()

https://codereview.chromium.org/287093002/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:2283: RenderViewHostImpl* host
= GetRenderManager()->current_host();
GetRenderViewHost()

https://codereview.chromium.org/287093002/diff/1/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/287093002/diff/1/content/renderer/render_view...
content/renderer/render_view_impl.cc:941:
render_view->webview()->mainFrame()->document().isPluginDocument();
This check is probably too early to know what we are loading. I'm not sure if
this makes sense.

[wjmaclean, 2014-05-16 01:57:08]

See my replies to your comments, but definitely let's chat about this tomorrow.

https://codereview.chromium.org/287093002/diff/1/content/browser/renderer_hos...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/287093002/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host_impl.cc:1624:
GetProcess()->GetID(), GetRoutingID(), zoom_level);
On 2014/05/15 23:26:53, Fady Samuel wrote:
> The flow of all this code can get pretty confusing pretty fast. I agree this
> needs to get fixed. I'd like to see diagrams tomorrow.

Agreed, it will be cleaner before it's done.

https://codereview.chromium.org/287093002/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/287093002/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:2282: BrowserPluginEmbedder*
embedder = GetBrowserPluginEmbedder();
On 2014/05/15 23:26:53, Fady Samuel wrote:
> If this is the main blocker to major code cleanup then let's not worry about
> this. Let's get rid of this and figure out an alternative solution elsewhere.
I
> want to get rid of WebContentsImpl::SetZoomLevel completely.

Yes, I'd like to simplify by doing the final removal of SetZoomLevel in a
separate CL.

https://codereview.chromium.org/287093002/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/287093002/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:2172: if
(GetRenderManager()->current_host()->uses_temporary_zoom_settings()) {
On 2014/05/15 23:26:53, Fady Samuel wrote:
> GetRenderViewHost()

Done.

https://codereview.chromium.org/287093002/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:2283: RenderViewHostImpl* host
= GetRenderManager()->current_host();
On 2014/05/15 23:26:53, Fady Samuel wrote:
> GetRenderViewHost()

Done.

https://codereview.chromium.org/287093002/diff/1/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/287093002/diff/1/content/renderer/render_view...
content/renderer/render_view_impl.cc:941:
render_view->webview()->mainFrame()->document().isPluginDocument();
On 2014/05/15 23:26:53, Fady Samuel wrote:
> This check is probably too early to know what we are loading. I'm not sure if
> this makes sense.

Actually, for a mainFrame document, I think this should be fine (I followed back
through the stack traces from createDocument()). But, what I'm more concerned
about, is if a mainFrame doc can change its pluginDocument status mid-stream.

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

https://codereview.chromium.org/287093002/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/287093002/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:2283: RenderViewHostImpl* host
= GetRenderManager()->current_host();
On 2014/05/16 01:57:08, wjmaclean wrote:
> On 2014/05/15 23:26:53, Fady Samuel wrote:
> > GetRenderViewHost()
> 
> Done.

Ooops, just realized that GetRenderViewHost() returns RenderViewHost*, and not
RenderViewHostImpl*. Is it better to use GetRenderViewHost() and do a static
cast?

[Fady Samuel, 2014-05-16 19:20:07]

https://codereview.chromium.org/287093002/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/287093002/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:2166: double
WebContentsImpl::GetZoomLevel() const {
Move this to HostZoomMap::GetZoomLevelForWebContents.

https://codereview.chromium.org/287093002/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:2282: BrowserPluginEmbedder*
embedder = GetBrowserPluginEmbedder();
On 2014/05/16 01:57:08, wjmaclean wrote:
> On 2014/05/15 23:26:53, Fady Samuel wrote:
> > If this is the main blocker to major code cleanup then let's not worry about
> > this. Let's get rid of this and figure out an alternative solution
elsewhere.
> I
> > want to get rid of WebContentsImpl::SetZoomLevel completely.
> 
> Yes, I'd like to simplify by doing the final removal of SetZoomLevel in a
> separate CL.

Can we do that in this CL? This CL doesn't seem particularly useful on its own.
It's not doing much code cleanup and it's not addressing all the issues of the
zoom API.

HostZoomMap::SetZoomLevelForWebContents.

https://codereview.chromium.org/287093002/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/287093002/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:2285: if
(host->uses_temporary_zoom_settings()) {
Can we move this flag to HostZoomMap?

https://codereview.chromium.org/287093002/diff/1/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/287093002/diff/1/content/renderer/render_view...
content/renderer/render_view_impl.cc:941:
render_view->webview()->mainFrame()->document().isPluginDocument();
On 2014/05/16 01:57:08, wjmaclean wrote:
> On 2014/05/15 23:26:53, Fady Samuel wrote:
> > This check is probably too early to know what we are loading. I'm not sure
if
> > this makes sense.
> 
> Actually, for a mainFrame document, I think this should be fine (I followed
back
> through the stack traces from createDocument()). But, what I'm more concerned
> about, is if a mainFrame doc can change its pluginDocument status mid-stream.

Yes, it can, at least for a <webview>. We keep around a RenderViewHost and
constantly navigate it. Maybe we can do the update here?
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...

[wjmaclean, 2014-05-16 19:32:48]

https://codereview.chromium.org/287093002/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (left):

https://codereview.chromium.org/287093002/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:2166: double
WebContentsImpl::GetZoomLevel() const {
On 2014/05/16 19:20:07, Fady Samuel wrote:
> Move this to HostZoomMap::GetZoomLevelForWebContents.

OK.

https://codereview.chromium.org/287093002/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:2282: BrowserPluginEmbedder*
embedder = GetBrowserPluginEmbedder();
On 2014/05/16 19:20:07, Fady Samuel wrote:
> On 2014/05/16 01:57:08, wjmaclean wrote:
> > On 2014/05/15 23:26:53, Fady Samuel wrote:
> > > If this is the main blocker to major code cleanup then let's not worry
about
> > > this. Let's get rid of this and figure out an alternative solution
> elsewhere.
> > I
> > > want to get rid of WebContentsImpl::SetZoomLevel completely.
> > 
> > Yes, I'd like to simplify by doing the final removal of SetZoomLevel in a
> > separate CL.
> 
> Can we do that in this CL?
> HostZoomMap::SetZoomLevelForWebContents.

Yes we can do that in this CL. I had just thought it would be simpler if we kept
that part of the refactoring separate.

> This CL doesn't seem particularly useful on its own.
> It's not doing much code cleanup and it's not addressing all the issues of the
> zoom API.

Not to be argumentative, but this CL *does* make a major change be removing the
cross-process synchronization issue. :-)

https://codereview.chromium.org/287093002/diff/1/content/browser/web_contents...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/287093002/diff/1/content/browser/web_contents...
content/browser/web_contents/web_contents_impl.cc:2285: if
(host->uses_temporary_zoom_settings()) {
On 2014/05/16 19:20:07, Fady Samuel wrote:
> Can we move this flag to HostZoomMap?

I'll look into this. Presumably it becomes part of the data stored in the map,
right?

https://codereview.chromium.org/287093002/diff/1/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/287093002/diff/1/content/renderer/render_view...
content/renderer/render_view_impl.cc:941:
render_view->webview()->mainFrame()->document().isPluginDocument();
On 2014/05/16 19:20:07, Fady Samuel wrote:
> On 2014/05/16 01:57:08, wjmaclean wrote:
> > On 2014/05/15 23:26:53, Fady Samuel wrote:
> > > This check is probably too early to know what we are loading. I'm not sure
> if
> > > this makes sense.
> > 
> > Actually, for a mainFrame document, I think this should be fine (I followed
> back
> > through the stack traces from createDocument()). But, what I'm more
concerned
> > about, is if a mainFrame doc can change its pluginDocument status
mid-stream.
> 
> Yes, it can, at least for a <webview>. We keep around a RenderViewHost and
> constantly navigate it. Maybe we can do the update here?
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
> 

Will investigate that.

[wjmaclean, 2014-05-21 20:59:12]

Here's a more up to date version of the CL. PTAL.

https://codereview.chromium.org/287093002/diff/1/content/renderer/render_view...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/287093002/diff/1/content/renderer/render_view...
content/renderer/render_view_impl.cc:941:
render_view->webview()->mainFrame()->document().isPluginDocument();
On 2014/05/16 19:20:07, Fady Samuel wrote:
> On 2014/05/16 01:57:08, wjmaclean wrote:
> > On 2014/05/15 23:26:53, Fady Samuel wrote:
> > > This check is probably too early to know what we are loading. I'm not sure
> if
> > > this makes sense.
> > 
> > Actually, for a mainFrame document, I think this should be fine (I followed
> back
> > through the stack traces from createDocument()). But, what I'm more
concerned
> > about, is if a mainFrame doc can change its pluginDocument status
mid-stream.
> 
> Yes, it can, at least for a <webview>. We keep around a RenderViewHost and
> constantly navigate it. Maybe we can do the update here?
>
https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...
> 

The way the temporary zoom settings are currently structured requires us to put
a renderer_process_id with each temporary setting. Could we do that in one of
the RenderViewHostImpl::Navigate* functions?
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/re...

[wjmaclean, 2014-05-22 18:52:44]

John - can you please take a look? We'd like to make this change to reduce some
of the IPCs that are making it hard to synchronize things in the new Tabs Zoom
API.

This CL also starts the task of trying to consolidate control over zoom
operations inside HostZoomMap.

[jam, 2014-05-23 15:51:03]

great that zooming is moving to one place.

pkasting: are you familiar with the details of this code to review?

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

https://codereview.chromium.org/287093002/diff/80001/content/browser/host_zoo...
content/browser/host_zoom_map_impl.h:44: const std::string& host);
here and above, don't put these methods which aren't part of the HostZoomMap
interface in this section

https://codereview.chromium.org/287093002/diff/80001/content/browser/host_zoo...
content/browser/host_zoom_map_impl.h:60: bool uses_temporary_zoom_settings);
nit: document everything. also new line after this

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

https://codereview.chromium.org/287093002/diff/80001/content/public/browser/h...
content/public/browser/host_zoom_map.h:60: double level);
nit: add comments about these methods

[wjmaclean, 2014-05-23 17:07:09]

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

https://codereview.chromium.org/287093002/diff/80001/content/browser/host_zoo...
content/browser/host_zoom_map_impl.h:44: const std::string& host);
On 2014/05/23 15:51:04, jam wrote:
> here and above, don't put these methods which aren't part of the HostZoomMap
> interface in this section

Ooops, not paying attention here ... will move these.

https://codereview.chromium.org/287093002/diff/80001/content/browser/host_zoo...
content/browser/host_zoom_map_impl.h:60: bool uses_temporary_zoom_settings);
On 2014/05/23 15:51:04, jam wrote:
> nit: document everything. also new line after this

Done.

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

https://codereview.chromium.org/287093002/diff/80001/content/public/browser/h...
content/public/browser/host_zoom_map.h:60: double level);
On 2014/05/23 15:51:04, jam wrote:
> nit: add comments about these methods

Done.

[Peter Kasting, 2014-05-28 02:17:30]

I will try to review this tomorrow.

[Peter Kasting, 2014-05-28 22:49:57]

So how do we now tell View B to change its zoom level when View A, which is
displaying the same host, changes its level? I thought that's what
ViewMsg_SetZoomLevel was doing, and I missed what's replacing that
functionality?

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

https://codereview.chromium.org/287093002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:217: static_cast<const
WebContentsImpl&>(web_contents);
These unconditional downcasts scare me.

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

https://codereview.chromium.org/287093002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:223: } else {
Nit: No else after return

https://codereview.chromium.org/287093002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:228: // the zoom level.
(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.

https://codereview.chromium.org/287093002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:229: url = entry ? entry->GetURL() :
GURL::EmptyGURL();
This use of EmptyGURL() is incorrect, and |url| is already empty anyway.  So do
this:

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

https://codereview.chromium.org/287093002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:265: TemporaryZoomLevel
zoom_level(render_process_id, render_view_id);
Tiny nit: You can move this line above the lock (just to absolutely minimize the
code inside locks)

https://codereview.chromium.org/287093002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:279: render_process_id, render_view_id,
default_zoom_level_);
Tiny nit: Again, this can go above the lock

https://codereview.chromium.org/287093002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:286: if (it !=
temporary_zoom_levels_.end())
Nit: Can be combined with the else

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

https://codereview.chromium.org/287093002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.h:50: // be a temporary zoom level, depending
on UsesTemporarySoomLevel().
Nit: Soom -> Zoom

https://codereview.chromium.org/287093002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.h:68: // from the host specification.
Nit: How about:

Returns whether the view manages its zoom level independently of other tabs
displaying content from the same host.

https://codereview.chromium.org/287093002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.h:73: // is true.
Nit: How about:

Sets whether the view manages its zoom level independently of other tabs
displaying content from the same host, based on whether
|uses_temporary_zoom_settings| is true.

https://codereview.chromium.org/287093002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.h:76: bool uses_temporary_zoom_settings);
Nit: I'd call this arg "...level" and not "...settings", for consistency with
the function names, and similarly for all other places in .h and .cc files that
use this name

https://codereview.chromium.org/287093002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.h:115: int render_process_id;
Nit: Members below functions

https://codereview.chromium.org/287093002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.h:122: zoom_level(level) {}
Nit: Since you have a .cc file anyway, I'd just move these definitions there
(based on the Chromium "don't inline unless you need to" rule)

https://codereview.chromium.org/287093002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.h:137: typedef
std::vector<TemporaryZoomLevel> TemporaryZoomLevelList;
Don't put "list" in the name, especially when the underlying type is not a list.

We would normally call this TemporaryZoomLevels.

https://codereview.chromium.org/287093002/diff/100001/content/common/view_mes...
File content/common/view_messages.h (right):

https://codereview.chromium.org/287093002/diff/100001/content/common/view_mes...
content/common/view_messages.h:1441: // zooming is handled differently for
plugin documents.
This change no longer seems to describe anywhere _why_ we do different things
for plugins, the way the old change did in render_view_impl.cc.  We need to put
such commentary somewhere appropriate or this just seems arbitrary.

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

https://codereview.chromium.org/287093002/diff/100001/content/public/browser/...
content/public/browser/host_zoom_map.h:59: // temporary or host specific.
Nit: host specific -> host-specific

https://codereview.chromium.org/287093002/diff/100001/content/public/browser/...
content/public/browser/host_zoom_map.h:63: // be temporary or hosti-specific
depending on the particular WebContents.
Nit: hosti -> host

[wjmaclean, 2014-05-29 14:49:44]

"So how do we now tell View B to change its zoom level when View A, which is
displaying the same host, changes its level? I thought that's what
ViewMsg_SetZoomLevel was doing, and I missed what's replacing that
functionality?"

Prior to this CL, ViewMsg_SetZoomLevel would tell the RenderView to change its
zoom level, then that RenderView would IPC back to HostZoomMap, which would then
IPC (with a different message) all matching RenderViews (if host-specific zoom
is in play). (I believe it was possible for the original RenderView to be
messaged multiple times.)

This CL talks to HostZoomMap *first*, and all matching RenderViews are updated
together. Does that help?

See comment-specific replies below.

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

https://codereview.chromium.org/287093002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:217: static_cast<const
WebContentsImpl&>(web_contents);
On 2014/05/28 22:49:58, Peter Kasting wrote:
> These unconditional downcasts scare me.
> 
> The API should be changed so that the functions that do this take a const
> WebContentsImpl& directly.

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.

https://codereview.chromium.org/287093002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:223: } else {
On 2014/05/28 22:49:58, Peter Kasting wrote:
> Nit: No else after return

Done.

https://codereview.chromium.org/287093002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:228: // the zoom level.
On 2014/05/28 22:49:58, Peter Kasting wrote:
> (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.

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.

https://codereview.chromium.org/287093002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:229: url = entry ? entry->GetURL() :
GURL::EmptyGURL();
On 2014/05/28 22:49:58, Peter Kasting wrote:
> This use of EmptyGURL() is incorrect, and |url| is already empty anyway.  So
do
> this:
> 
>   if (entry)
>     url = entry->GetURL();

Done.

https://codereview.chromium.org/287093002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:265: TemporaryZoomLevel
zoom_level(render_process_id, render_view_id);
On 2014/05/28 22:49:58, Peter Kasting wrote:
> Tiny nit: You can move this line above the lock (just to absolutely minimize
the
> code inside locks)

Done.

https://codereview.chromium.org/287093002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:279: render_process_id, render_view_id,
default_zoom_level_);
On 2014/05/28 22:49:58, Peter Kasting wrote:
> Tiny nit: Again, this can go above the lock

Done.

https://codereview.chromium.org/287093002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:286: if (it !=
temporary_zoom_levels_.end())
On 2014/05/28 22:49:58, Peter Kasting wrote:
> Nit: Can be combined with the else

Done.

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

https://codereview.chromium.org/287093002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.h:50: // be a temporary zoom level, depending
on UsesTemporarySoomLevel().
On 2014/05/28 22:49:58, Peter Kasting wrote:
> Nit: Soom -> Zoom

Done.

https://codereview.chromium.org/287093002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.h:68: // from the host specification.
On 2014/05/28 22:49:58, Peter Kasting wrote:
> Nit: How about:
> 
> Returns whether the view manages its zoom level independently of other tabs
> displaying content from the same host.

Sure, sounds good.

https://codereview.chromium.org/287093002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.h:73: // is true.
On 2014/05/28 22:49:58, Peter Kasting wrote:
> Nit: How about:
> 
> Sets whether the view manages its zoom level independently of other tabs
> displaying content from the same host, based on whether
> |uses_temporary_zoom_settings| is true.

Sure, good wording!

https://codereview.chromium.org/287093002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.h:76: bool uses_temporary_zoom_settings);
On 2014/05/28 22:49:58, Peter Kasting wrote:
> Nit: I'd call this arg "...level" and not "...settings", for consistency with
> the function names, and similarly for all other places in .h and .cc files
that
> use this name

Done.

https://codereview.chromium.org/287093002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.h:115: int render_process_id;
On 2014/05/28 22:49:58, Peter Kasting wrote:
> Nit: Members below functions

Done.

https://codereview.chromium.org/287093002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.h:122: zoom_level(level) {}
On 2014/05/28 22:49:58, Peter Kasting wrote:
> Nit: Since you have a .cc file anyway, I'd just move these definitions there
> (based on the Chromium "don't inline unless you need to" rule)

Done.

https://codereview.chromium.org/287093002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.h:137: typedef
std::vector<TemporaryZoomLevel> TemporaryZoomLevelList;
On 2014/05/28 22:49:58, Peter Kasting wrote:
> Don't put "list" in the name, especially when the underlying type is not a
list.
> 
> We would normally call this TemporaryZoomLevels.

Done.

https://codereview.chromium.org/287093002/diff/100001/content/common/view_mes...
File content/common/view_messages.h (right):

https://codereview.chromium.org/287093002/diff/100001/content/common/view_mes...
content/common/view_messages.h:1441: // zooming is handled differently for
plugin documents.
On 2014/05/28 22:49:58, Peter Kasting wrote:
> This change no longer seems to describe anywhere _why_ we do different things
> for plugins, the way the old change did in render_view_impl.cc.  We need to
put
> such commentary somewhere appropriate or this just seems arbitrary.

I've added this comment to the callsites that check isPluginDocument().

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

https://codereview.chromium.org/287093002/diff/100001/content/public/browser/...
content/public/browser/host_zoom_map.h:59: // temporary or host specific.
On 2014/05/28 22:49:58, Peter Kasting wrote:
> Nit: host specific -> host-specific

Done.

https://codereview.chromium.org/287093002/diff/100001/content/public/browser/...
content/public/browser/host_zoom_map.h:63: // be temporary or hosti-specific
depending on the particular WebContents.
On 2014/05/28 22:49:58, Peter Kasting wrote:
> Nit: hosti -> host

Done.

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

> This CL talks to HostZoomMap *first*, and all matching RenderViews are updated
> together. Does that help?

As long as everyone gets updated, I'm fine.  It's just been so long since I
worked with this that I no longer recognize the code that updates all the other
RenderViews.

LGTM

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

https://codereview.chromium.org/287093002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:217: static_cast<const
WebContentsImpl&>(web_contents);
On 2014/05/29 14:49:45, wjmaclean wrote:
> On 2014/05/28 22:49:58, Peter Kasting wrote:
> > These unconditional downcasts scare me.
> > 
> > The API should be changed so that the functions that do this take a const
> > WebContentsImpl& directly.
> 
> 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.

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.

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

https://codereview.chromium.org/287093002/diff/120001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:373: : render_process_id(process_id),
render_view_id(view_id), zoom_level(0.0) {
Nit: I read the style guide as saying that these need to be one per line when
not all on the line with the function name (but not everyone agrees with this
interpretation) -- doing this would also look more consistent with the function
above

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

https://codereview.chromium.org/287093002/diff/120001/content/browser/host_zo...
content/browser/host_zoom_map_impl.h:128: typedef
std::vector<TemporaryZoomLevel> TemporaryZoomLevels;
Nit: Style guide says typedefs and enums go at the top of the section (this also
applies to the two typedefs higher up)

[wjmaclean, 2014-05-29 20:14:19]

I'm happy to look at the issue of exposing these APIs through the base class
(perhaps in a separate CL). jam@, did you have any thoughts about that?

kenrb@ - can you provide an OWNERS review for view_messages.h?

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

https://codereview.chromium.org/287093002/diff/120001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:373: : render_process_id(process_id),
render_view_id(view_id), zoom_level(0.0) {
On 2014/05/29 18:16:08, Peter Kasting wrote:
> Nit: I read the style guide as saying that these need to be one per line when
> not all on the line with the function name (but not everyone agrees with this
> interpretation) -- doing this would also look more consistent with the
function
> above

Done.

Good point ... this was auto-formatted, so I wasn't sure I should override it,
but I agree about the consistency.

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

https://codereview.chromium.org/287093002/diff/120001/content/browser/host_zo...
content/browser/host_zoom_map_impl.h:128: typedef
std::vector<TemporaryZoomLevel> TemporaryZoomLevels;
On 2014/05/29 18:16:08, Peter Kasting wrote:
> Nit: Style guide says typedefs and enums go at the top of the section (this
also
> applies to the two typedefs higher up)

Done.

[kenrb, 2014-05-29 20:25:31]

lgtm for IPC change

[wjmaclean, 2014-05-29 20:27:49]

The CQ bit was checked by wjmaclean@chromium.org

[fsamuel, 2014-05-29 20:28:57]

https://codereview.chromium.org/287093002/diff/140001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/287093002/diff/140001/content/renderer/render...
content/renderer/render_view_impl.cc:948: render_view->Send(new
ViewHostMsg_DidCreateDocument(
Given you pass this information about plugin documents in render_frame_impl, do
you need this?

[wjmaclean, 2014-05-29 20:31:09]

The CQ bit was unchecked by wjmaclean@chromium.org

[wjmaclean, 2014-05-29 20:52:29]

https://codereview.chromium.org/287093002/diff/140001/content/renderer/render...
File content/renderer/render_view_impl.cc (right):

https://codereview.chromium.org/287093002/diff/140001/content/renderer/render...
content/renderer/render_view_impl.cc:948: render_view->Send(new
ViewHostMsg_DidCreateDocument(
On 2014/05/29 20:28:57, fsamuel wrote:
> Given you pass this information about plugin documents in render_frame_impl,
do
> you need this?

We can try without ... PDFs don't seem to need it.

[wjmaclean, 2014-05-29 20:52:38]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-05-29 20:54:49]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wjmaclean@chromium.org/287093002/160001

[commit-bot: I haz the power, 2014-05-30 03:30:25]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/18...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-05-30 03:37:59]

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

[commit-bot: I haz the power, 2014-05-30 03:37:59]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[jam, 2014-05-30 06:47:43]

lgtm

i defer to peter's review of the details of the zoom changes

[wjmaclean, 2014-05-30 13:17:41]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-05-30 13:18:58]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wjmaclean@chromium.org/287093002/180001

[commit-bot: I haz the power, 2014-05-30 14:58:15]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[commit-bot: I haz the power, 2014-05-30 14:59:09]

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

[commit-bot: I haz the power, 2014-05-30 14:59:10]

Try jobs failed on following builders:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)

[wjmaclean, 2014-05-30 15:01:31]

The CQ bit was checked by wjmaclean@chromium.org

[commit-bot: I haz the power, 2014-05-30 15:02:27]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/wjmaclean@chromium.org/287093002/180001

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

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

https://codereview.chromium.org/287093002/diff/100001/content/browser/host_zo...
content/browser/host_zoom_map_impl.cc:217: static_cast<const
WebContentsImpl&>(web_contents);
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.

[commit-bot: I haz the power, 2014-05-30 16:24:36]

Change committed as 273854