Remove usage of HostZoomMap from c/b/loader via ReadyToCommitNavigation

Remove usage of HostZoomMap from c/b/loader via ReadyToCommitNavigation

Had to pull the mojom out into a separate interface because Frame
itself isn't associated, so a SetHostZoomLevel on Frame would be racy
with the FrameMsg_CommitNavigation.

Also fixes the IFrameZoomBrowserTest under PlzNavigate because it uses
the new machinery.

R=nasko@chromium.org
BUG=609607
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14
Cr-Commit-Position: refs/heads/master@{#428084}

[scottmg, 2016-09-27 22:44:01]

Description was changed from

==========
Remove usage of HostZoomMap from c/b/loader via ReadyToCommitNavigation

Doesn't work, racy between Frame message and actual commit.

R=nasko@chromium.org
BUG=609607
==========

to

==========
Remove usage of HostZoomMap from c/b/loader via ReadyToCommitNavigation

Doesn't work, racy between Frame message and actual commit.

R=nasko@chromium.org
BUG=609607
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[nasko, 2016-09-28 01:32:05]

What part of it is racy? Is it in PlzNavigate mode, current mode, or both?

https://codereview.chromium.org/2374863002/diff/20001/content/browser/host_zo...
File content/browser/host_zoom_map_host.cc (right):

https://codereview.chromium.org/2374863002/diff/20001/content/browser/host_zo...
content/browser/host_zoom_map_host.cc:26: RenderProcessHost* rph =
render_frame_host_impl_->GetProcess();
Shouldn't this be done only for the main frame? I thought we don't care about
the zoom level for subframes.

https://codereview.chromium.org/2374863002/diff/20001/content/browser/host_zo...
File content/browser/host_zoom_map_host.h (right):

https://codereview.chromium.org/2374863002/diff/20001/content/browser/host_zo...
content/browser/host_zoom_map_host.h:16: class CONTENT_EXPORT HostZoomMapHost :
private WebContentsObserver {
I find the Host suffix a bit confusing, as this object is just an observer.

https://codereview.chromium.org/2374863002/diff/20001/content/browser/host_zo...
content/browser/host_zoom_map_host.h:19: RenderFrameHostImpl*
render_frame_host_impl);
nit: No need for _impl, we skip that on a lot of param names inside content.

[scottmg, 2016-09-28 01:52:39]

On 2016/09/28 01:32:05, nasko (slow) wrote:
> What part of it is racy? Is it in PlzNavigate mode, current mode, or both?

Thanks for looking! I *think* the race is that the mojo message that's sent here
(in ps#1) isn't associated with the channel that's used for
FrameMsg_CommitNavigation, so even though we send it earlier on the browser
side, we don't have a guarantee that it gets to the renderer before the IPC
ones.

In practice when creating a new window occasionally the content is rendered
without the correct zoom level, but the magnifying glass in the omnibox "knows"
the correct zoom level. So if the right level is say 150%, then zooming up one
notch goes from 100% to 175%.

(I started in ps#2 to try splitting it out as a separate new interface so it
could be ordered using the AssociatedInterface goop, but that's crashing at
registration time so I guess there's some ordering startup problem. Haven't had
a chance to figure that out yet.)

> 
>
https://codereview.chromium.org/2374863002/diff/20001/content/browser/host_zo...
> File content/browser/host_zoom_map_host.cc (right):
> 
>
https://codereview.chromium.org/2374863002/diff/20001/content/browser/host_zo...
> content/browser/host_zoom_map_host.cc:26: RenderProcessHost* rph =
> render_frame_host_impl_->GetProcess();
> Shouldn't this be done only for the main frame? I thought we don't care about
> the zoom level for subframes.
> 
>
https://codereview.chromium.org/2374863002/diff/20001/content/browser/host_zo...
> File content/browser/host_zoom_map_host.h (right):
> 
>
https://codereview.chromium.org/2374863002/diff/20001/content/browser/host_zo...
> content/browser/host_zoom_map_host.h:16: class CONTENT_EXPORT HostZoomMapHost
:
> private WebContentsObserver {
> I find the Host suffix a bit confusing, as this object is just an observer.
> 
>
https://codereview.chromium.org/2374863002/diff/20001/content/browser/host_zo...
> content/browser/host_zoom_map_host.h:19: RenderFrameHostImpl*
> render_frame_host_impl);
> nit: No need for _impl, we skip that on a lot of param names inside content.

[scottmg, 2016-09-28 02:40:13]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 02:40:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[scottmg, 2016-09-28 02:41:29]

Description was changed from

==========
Remove usage of HostZoomMap from c/b/loader via ReadyToCommitNavigation

Doesn't work, racy between Frame message and actual commit.

R=nasko@chromium.org
BUG=609607
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove usage of HostZoomMap from c/b/loader via ReadyToCommitNavigation

Had to pull the mojom out into a separate interface because Frame
itself isn't associated, so a SetHostZoomLevel on Frame would be racy
with the FrameMsg_Commit.

Also fixes the IFrameZoomBrowserTest under PlzNavigate because it uses
the new machinery.

R=nasko@chromium.org
BUG=609607
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[scottmg, 2016-09-28 02:44:53]

This seems to work reasonably now. I don't love that HostZoom is its own thing,
but I think it's necessary as opposed to having it on Frame.mojom to get the
ordering correct.

https://codereview.chromium.org/2374863002/diff/20001/content/browser/host_zo...
File content/browser/host_zoom_map_host.cc (right):

https://codereview.chromium.org/2374863002/diff/20001/content/browser/host_zo...
content/browser/host_zoom_map_host.cc:26: RenderProcessHost* rph =
render_frame_host_impl_->GetProcess();
On 2016/09/28 01:32:04, nasko (slow) wrote:
> Shouldn't this be done only for the main frame? I thought we don't care about
> the zoom level for subframes.

Done.

https://codereview.chromium.org/2374863002/diff/20001/content/browser/host_zo...
File content/browser/host_zoom_map_host.h (right):

https://codereview.chromium.org/2374863002/diff/20001/content/browser/host_zo...
content/browser/host_zoom_map_host.h:16: class CONTENT_EXPORT HostZoomMapHost :
private WebContentsObserver {
On 2016/09/28 01:32:04, nasko (slow) wrote:
> I find the Host suffix a bit confusing, as this object is just an observer.

Yeah. :( Sender? Observer?

https://codereview.chromium.org/2374863002/diff/20001/content/browser/host_zo...
content/browser/host_zoom_map_host.h:19: RenderFrameHostImpl*
render_frame_host_impl);
On 2016/09/28 01:32:04, nasko (slow) wrote:
> nit: No need for _impl, we skip that on a lot of param names inside content.

Done.

[commit-bot: I haz the power, 2016-09-28 02:53:30]

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

[commit-bot: I haz the power, 2016-09-28 02:53:31]

Dry run: Try jobs failed on following builders:
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[scottmg, 2016-09-28 03:40:44]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 03:41:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 04:10:02]

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

[commit-bot: I haz the power, 2016-09-28 04:10:03]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[scottmg, 2016-09-28 16:30:09]

On 2016/09/28 02:44:53, scottmg wrote:
> This seems to work reasonably now. I don't love that HostZoom is its own
thing,
> but I think it's necessary as opposed to having it on Frame.mojom to get the
> ordering correct.

There's one test failing (WebViewTests/WebViewTest.ReloadEmbedder/1) but in
debugging it, I'm not sure what's going on or whether I've set this up right at
all. So ignore this for now and I'll ping again once I've figured out what's
going on.

> 
>
https://codereview.chromium.org/2374863002/diff/20001/content/browser/host_zo...
> File content/browser/host_zoom_map_host.cc (right):
> 
>
https://codereview.chromium.org/2374863002/diff/20001/content/browser/host_zo...
> content/browser/host_zoom_map_host.cc:26: RenderProcessHost* rph =
> render_frame_host_impl_->GetProcess();
> On 2016/09/28 01:32:04, nasko (slow) wrote:
> > Shouldn't this be done only for the main frame? I thought we don't care
about
> > the zoom level for subframes.
> 
> Done.
> 
>
https://codereview.chromium.org/2374863002/diff/20001/content/browser/host_zo...
> File content/browser/host_zoom_map_host.h (right):
> 
>
https://codereview.chromium.org/2374863002/diff/20001/content/browser/host_zo...
> content/browser/host_zoom_map_host.h:16: class CONTENT_EXPORT HostZoomMapHost
:
> private WebContentsObserver {
> On 2016/09/28 01:32:04, nasko (slow) wrote:
> > I find the Host suffix a bit confusing, as this object is just an observer.
> 
> Yeah. :( Sender? Observer?
> 
>
https://codereview.chromium.org/2374863002/diff/20001/content/browser/host_zo...
> content/browser/host_zoom_map_host.h:19: RenderFrameHostImpl*
> render_frame_host_impl);
> On 2016/09/28 01:32:04, nasko (slow) wrote:
> > nit: No need for _impl, we skip that on a lot of param names inside content.
> 
> Done.

[scottmg, 2016-09-28 17:23:25]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 17:23:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[scottmg, 2016-09-28 17:27:10]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 17:27:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 17:50:52]

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

[commit-bot: I haz the power, 2016-09-28 17:50:53]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[scottmg, 2016-09-28 18:29:14]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 18:29:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 18:56:01]

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

[commit-bot: I haz the power, 2016-09-28 18:56:02]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[scottmg, 2016-09-28 19:30:47]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 19:31:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 20:13:06]

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

[commit-bot: I haz the power, 2016-09-28 20:13:06]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[scottmg, 2016-09-28 20:28:02]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 20:28:26]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[scottmg, 2016-09-28 21:27:30]

On 2016/09/28 16:30:09, scottmg wrote:
> On 2016/09/28 02:44:53, scottmg wrote:
> > This seems to work reasonably now. I don't love that HostZoom is its own
> thing,
> > but I think it's necessary as opposed to having it on Frame.mojom to get the
> > ordering correct.
> 
> There's one test failing (WebViewTests/WebViewTest.ReloadEmbedder/1) but in
> debugging it, I'm not sure what's going on or whether I've set this up right
at
> all. So ignore this for now and I'll ping again once I've figured out what's
> going on.

This seems to be poked into working now, so please take a look when you have
some time.

> 
> > 
> >
>
https://codereview.chromium.org/2374863002/diff/20001/content/browser/host_zo...
> > File content/browser/host_zoom_map_host.cc (right):
> > 
> >
>
https://codereview.chromium.org/2374863002/diff/20001/content/browser/host_zo...
> > content/browser/host_zoom_map_host.cc:26: RenderProcessHost* rph =
> > render_frame_host_impl_->GetProcess();
> > On 2016/09/28 01:32:04, nasko (slow) wrote:
> > > Shouldn't this be done only for the main frame? I thought we don't care
> about
> > > the zoom level for subframes.
> > 
> > Done.
> > 
> >
>
https://codereview.chromium.org/2374863002/diff/20001/content/browser/host_zo...
> > File content/browser/host_zoom_map_host.h (right):
> > 
> >
>
https://codereview.chromium.org/2374863002/diff/20001/content/browser/host_zo...
> > content/browser/host_zoom_map_host.h:16: class CONTENT_EXPORT
HostZoomMapHost
> :
> > private WebContentsObserver {
> > On 2016/09/28 01:32:04, nasko (slow) wrote:
> > > I find the Host suffix a bit confusing, as this object is just an
observer.
> > 
> > Yeah. :( Sender? Observer?
> > 
> >
>
https://codereview.chromium.org/2374863002/diff/20001/content/browser/host_zo...
> > content/browser/host_zoom_map_host.h:19: RenderFrameHostImpl*
> > render_frame_host_impl);
> > On 2016/09/28 01:32:04, nasko (slow) wrote:
> > > nit: No need for _impl, we skip that on a lot of param names inside
content.
> > 
> > Done.

[commit-bot: I haz the power, 2016-09-28 21:38:04]

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

[commit-bot: I haz the power, 2016-09-28 21:38:05]

Dry run: This issue passed the CQ dry run.

[scottmg, 2016-09-28 23:50:52]

Description was changed from

==========
Remove usage of HostZoomMap from c/b/loader via ReadyToCommitNavigation

Had to pull the mojom out into a separate interface because Frame
itself isn't associated, so a SetHostZoomLevel on Frame would be racy
with the FrameMsg_Commit.

Also fixes the IFrameZoomBrowserTest under PlzNavigate because it uses
the new machinery.

R=nasko@chromium.org
BUG=609607
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove usage of HostZoomMap from c/b/loader via ReadyToCommitNavigation

Had to pull the mojom out into a separate interface because Frame
itself isn't associated, so a SetHostZoomLevel on Frame would be racy
with the FrameMsg_CommitNavigation.

Also fixes the IFrameZoomBrowserTest under PlzNavigate because it uses
the new machinery.

R=nasko@chromium.org
BUG=609607
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[nasko, 2016-10-17 22:49:03]

Now that ReadyToCommitNavigation is looking like it will stick around, I think
this one is ready to depend on it.

Just a couple of nits and one question, otherwise it looks much better than
before. Thanks!

https://codereview.chromium.org/2374863002/diff/160001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2374863002/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:315:
HostZoomMapObserver::CreateForWebContents(delegate->GetAsWebContents());
Why create the observer here? If it is per WebContents, it should be created in
the WebContents constructor, no?

https://codereview.chromium.org/2374863002/diff/160001/content/renderer/rende...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2374863002/diff/160001/content/renderer/rende...
content/renderer/render_frame_impl.h:457: 
nit: Do we need the empty line here?

https://codereview.chromium.org/2374863002/diff/160001/content/renderer/rende...
content/renderer/render_frame_impl.h:1306:
mojo::AssociatedBinding<mojom::HostZoom> host_zoom_binding_;
nit: Move it above the FrameHostPtr, so all mojo::*Binding are grouped together.

[scottmg, 2016-10-25 19:57:53]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 19:58:23]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[scottmg, 2016-10-25 20:12:40]

Thanks! Sorry for the laggard turnaround time.

https://codereview.chromium.org/2374863002/diff/160001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/2374863002/diff/160001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.cc:315:
HostZoomMapObserver::CreateForWebContents(delegate->GetAsWebContents());
On 2016/10/17 22:49:03, nasko (slow) wrote:
> Why create the observer here? If it is per WebContents, it should be created
in
> the WebContents constructor, no?

Why, indeed. Done.

https://codereview.chromium.org/2374863002/diff/160001/content/renderer/rende...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/2374863002/diff/160001/content/renderer/rende...
content/renderer/render_frame_impl.h:457: 
On 2016/10/17 22:49:03, nasko (slow) wrote:
> nit: Do we need the empty line here?

It's not part of mojom::HostZoom. Moved it up near RegisterMojoInterfaces()
instead.

https://codereview.chromium.org/2374863002/diff/160001/content/renderer/rende...
content/renderer/render_frame_impl.h:1306:
mojo::AssociatedBinding<mojom::HostZoom> host_zoom_binding_;
On 2016/10/17 22:49:03, nasko (slow) wrote:
> nit: Move it above the FrameHostPtr, so all mojo::*Binding are grouped
together.

Done.

[commit-bot: I haz the power, 2016-10-25 21:09:05]

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

[commit-bot: I haz the power, 2016-10-25 21:09:06]

Dry run: Try jobs failed on following builders:
  linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)

[nasko, 2016-10-25 23:17:08]

LGTM with some minor nits.

https://codereview.chromium.org/2374863002/diff/200001/content/browser/host_z...
File content/browser/host_zoom_map_observer.cc (right):

https://codereview.chromium.org/2374863002/diff/200001/content/browser/host_z...
content/browser/host_zoom_map_observer.cc:23: if
(navigation_handle->IsInMainFrame()) {
minor nit: Early returns make the code easier to read and no need for indents.
Maybe inverse the check and return?

https://codereview.chromium.org/2374863002/diff/200001/content/browser/host_z...
content/browser/host_zoom_map_observer.cc:24: if (host_zoom_.is_bound()) {
nit: Should this ever not be bound? If not, maybe a DCHECK?

https://codereview.chromium.org/2374863002/diff/200001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2374863002/diff/200001/content/renderer/rende...
content/renderer/render_frame_impl.cc:6431:
GetAssociatedInterfaceRegistry()->AddInterface(base::Bind(
nit: Add a comment that the host zoom is per-page, so only for the main frame.

[scottmg, 2016-10-25 23:30:00]

Thanks!

https://codereview.chromium.org/2374863002/diff/200001/content/browser/host_z...
File content/browser/host_zoom_map_observer.cc (right):

https://codereview.chromium.org/2374863002/diff/200001/content/browser/host_z...
content/browser/host_zoom_map_observer.cc:23: if
(navigation_handle->IsInMainFrame()) {
On 2016/10/25 23:17:07, nasko (slow) wrote:
> minor nit: Early returns make the code easier to read and no need for indents.
> Maybe inverse the check and return?

Done x2.

https://codereview.chromium.org/2374863002/diff/200001/content/browser/host_z...
content/browser/host_zoom_map_observer.cc:24: if (host_zoom_.is_bound()) {
On 2016/10/25 23:17:07, nasko (slow) wrote:
> nit: Should this ever not be bound? If not, maybe a DCHECK?

Added a DCHECK for tryruns. I think I might have added it for some tests that do
weird setup, but it's been too long and I forget. If possible, I'll land with
the dcheck.

https://codereview.chromium.org/2374863002/diff/200001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2374863002/diff/200001/content/renderer/rende...
content/renderer/render_frame_impl.cc:6431:
GetAssociatedInterfaceRegistry()->AddInterface(base::Bind(
On 2016/10/25 23:17:07, nasko (slow) wrote:
> nit: Add a comment that the host zoom is per-page, so only for the main frame.

Done.

[scottmg, 2016-10-25 23:31:24]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-25 23:32:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 00:06:35]

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

[commit-bot: I haz the power, 2016-10-26 00:06:36]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[scottmg, 2016-10-26 00:23:02]

https://codereview.chromium.org/2374863002/diff/200001/content/browser/host_z...
File content/browser/host_zoom_map_observer.cc (right):

https://codereview.chromium.org/2374863002/diff/200001/content/browser/host_z...
content/browser/host_zoom_map_observer.cc:24: if (host_zoom_.is_bound()) {
On 2016/10/25 23:30:00, scottmg wrote:
> On 2016/10/25 23:17:07, nasko (slow) wrote:
> > nit: Should this ever not be bound? If not, maybe a DCHECK?
> 
> Added a DCHECK for tryruns. I think I might have added it for some tests that
do
> weird setup, but it's been too long and I forget. If possible, I'll land with
> the dcheck.

It looks like there's "only" 2 content_unittests that fail with the DCHECK. I'll
take a look to see if I can easily set up the environment enough to satisfy
them, otherwise will remove the DCHECK to land.

[scottmg, 2016-10-26 22:09:12]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 22:09:42]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-26 22:45:42]

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

[commit-bot: I haz the power, 2016-10-26 22:45:43]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[scottmg, 2016-10-27 00:35:00]

Patchset #13 (id:240001) has been deleted

[scottmg, 2016-10-27 00:37:03]

The CQ bit was checked by scottmg@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-27 00:37:37]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[scottmg, 2016-10-27 01:55:00]

On 2016/10/26 00:23:02, scottmg wrote:
>
https://codereview.chromium.org/2374863002/diff/200001/content/browser/host_z...
> File content/browser/host_zoom_map_observer.cc (right):
> 
>
https://codereview.chromium.org/2374863002/diff/200001/content/browser/host_z...
> content/browser/host_zoom_map_observer.cc:24: if (host_zoom_.is_bound()) {
> On 2016/10/25 23:30:00, scottmg wrote:
> > On 2016/10/25 23:17:07, nasko (slow) wrote:
> > > nit: Should this ever not be bound? If not, maybe a DCHECK?
> > 
> > Added a DCHECK for tryruns. I think I might have added it for some tests
that
> do
> > weird setup, but it's been too long and I forget. If possible, I'll land
with
> > the dcheck.
> 
> It looks like there's "only" 2 content_unittests that fail with the DCHECK.
I'll
> take a look to see if I can easily set up the environment enough to satisfy
> them, otherwise will remove the DCHECK to land.

(Turns out it was 2 content_unittests and 1 unit_tests to make that DCHECK work.
Diff is ps#11 to ps#12 if you want to eyeball it.)

[scottmg, 2016-10-27 01:57:23]

The CQ bit was unchecked by scottmg@chromium.org

[scottmg, 2016-10-27 01:57:57]

The CQ bit was checked by scottmg@chromium.org

[scottmg, 2016-10-27 01:57:58]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/2374863002/#ps260001
(title: "init")

[commit-bot: I haz the power, 2016-10-27 01:58:16]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[scottmg, 2016-10-27 02:02:31]

scottmg@chromium.org changed reviewers:
+ bengr@chromium.org

[scottmg, 2016-10-27 02:03:04]

+bengr for chrome/browser/previews/OWNERS.

[commit-bot: I haz the power, 2016-10-27 02:06:13]

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

[commit-bot: I haz the power, 2016-10-27 02:06:16]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[nasko, 2016-10-27 14:01:28]

https://codereview.chromium.org/2374863002/diff/200001/content/browser/host_z...
File content/browser/host_zoom_map_observer.cc (right):

https://codereview.chromium.org/2374863002/diff/200001/content/browser/host_z...
content/browser/host_zoom_map_observer.cc:24: if (host_zoom_.is_bound()) {
On 2016/10/26 00:23:02, scottmg wrote:
> On 2016/10/25 23:30:00, scottmg wrote:
> > On 2016/10/25 23:17:07, nasko (slow) wrote:
> > > nit: Should this ever not be bound? If not, maybe a DCHECK?
> > 
> > Added a DCHECK for tryruns. I think I might have added it for some tests
that
> do
> > weird setup, but it's been too long and I forget. If possible, I'll land
with
> > the dcheck.
> 
> It looks like there's "only" 2 content_unittests that fail with the DCHECK.
I'll
> take a look to see if I can easily set up the environment enough to satisfy
> them, otherwise will remove the DCHECK to land.

Awesome! Thanks for fixing those to keep the DCHECK in.

[bengr, 2016-10-27 18:13:52]

chrome/browser/previews/* lgtm

[scottmg, 2016-10-27 18:19:33]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2016-10-27 18:20:15]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-27 18:25:43]

Description was changed from

==========
Remove usage of HostZoomMap from c/b/loader via ReadyToCommitNavigation

Had to pull the mojom out into a separate interface because Frame
itself isn't associated, so a SetHostZoomLevel on Frame would be racy
with the FrameMsg_CommitNavigation.

Also fixes the IFrameZoomBrowserTest under PlzNavigate because it uses
the new machinery.

R=nasko@chromium.org
BUG=609607
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove usage of HostZoomMap from c/b/loader via ReadyToCommitNavigation

Had to pull the mojom out into a separate interface because Frame
itself isn't associated, so a SetHostZoomLevel on Frame would be racy
with the FrameMsg_CommitNavigation.

Also fixes the IFrameZoomBrowserTest under PlzNavigate because it uses
the new machinery.

R=nasko@chromium.org
BUG=609607
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-10-27 18:25:45]

Committed patchset #13 (id:260001)

[commit-bot: I haz the power, 2016-10-27 18:30:54]

Description was changed from

==========
Remove usage of HostZoomMap from c/b/loader via ReadyToCommitNavigation

Had to pull the mojom out into a separate interface because Frame
itself isn't associated, so a SetHostZoomLevel on Frame would be racy
with the FrameMsg_CommitNavigation.

Also fixes the IFrameZoomBrowserTest under PlzNavigate because it uses
the new machinery.

R=nasko@chromium.org
BUG=609607
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Remove usage of HostZoomMap from c/b/loader via ReadyToCommitNavigation

Had to pull the mojom out into a separate interface because Frame
itself isn't associated, so a SetHostZoomLevel on Frame would be racy
with the FrameMsg_CommitNavigation.

Also fixes the IFrameZoomBrowserTest under PlzNavigate because it uses
the new machinery.

R=nasko@chromium.org
BUG=609607
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14
Cr-Commit-Position: refs/heads/master@{#428084}
==========

[commit-bot: I haz the power, 2016-10-27 18:30:56]

Patchset 13 (id:??) landed as
https://crrev.com/276753cf9b79b05acdc4a262241b8b31bf212e14
Cr-Commit-Position: refs/heads/master@{#428084}