Thanks for bisecting. I thought we'd sorted out the Mojo ordering on this, but I guess not. :(

HostZoomMapObserver::ReadyToCommitNavigation always seems to call SetHostZoomLevel() here https://cs.chromium.org/chromium/src/content/browser/host_zoom_map_observer.cc?dr=CSs&q=SetHostZoomLevel+HostZoomMapObserver::ReadyToCommitNavigation&l=38 at the appropriate time.

Subsequently (i.e. correctly after the above) content::RenderFrameImpl::SendDidCommitProvisionalLoad checks to see what the zoom level is and sets it.

But RenderFrameImpl::SetHostZoomLevel in the renderer rarely gets called, so the renderer doesn't see the zoom level. I don't know why that is yet.

host_zoom_.is_bound() is true on the browser side.

Hmm, the behaviour in #3 is only in --single-process. (I don't know why that should be though.)

In normal process mode it seems harder to reproduce. I haven't seen it with link, back, but I have occasionally with just opening lots of news.yc tabs.

I suspect the two are different causes so I'm going to ignore the single-process case for now.

The function calls from a failure followed by a case that worked.

FAILED:

[22648:9172:1214/145533.270:ERROR:host_zoom_map_observer.cc(42)] content::HostZoomMapObserver::ReadyToCommitNavigation browser: https://www.google.ca/_/chrome/newtab?rlz=1C1CHBF_enCA711CA711&espv=2&ie=UTF-8 == 0
[20524:7072:1214/145533.271:ERROR:render_frame_impl.cc(2679)] content::RenderFrameImpl::SetHostZoomLevel: https://www.google.ca/_/chrome/newtab?rlz=1C1CHBF_enCA711CA711&espv=2&ie=UTF-8 == 0
[20524:7072:1214/145533.303:ERROR:render_frame_impl.cc(4857)] content::RenderFrameImpl::SendDidCommitProvisionalLoad  first case: 0
[20524:7072:1214/145533.303:ERROR:render_frame_impl.cc(4869)] content::RenderFrameImpl::SendDidCommitProvisionalLoad  deleted
[22648:9172:1214/145533.536:ERROR:host_zoom_map_observer.cc(48)] content::HostZoomMapObserver::RenderFrameCreated
[10296:6472:1214/145533.595:ERROR:render_frame_impl.cc(6509)] content::RenderFrameImpl::RegisterMojoInterfaces
[10296:6472:1214/145533.637:ERROR:render_frame_impl.cc(6523)] content::RenderFrameImpl::OnHostZoomClientRequest
[22648:9172:1214/145533.922:ERROR:host_zoom_map_observer.cc(48)] content::HostZoomMapObserver::RenderFrameCreated
[22648:9172:1214/145533.945:ERROR:host_zoom_map_observer.cc(24)] ReadyToCommitNavigation, not main frame
[20984:9408:1214/145534.399:ERROR:render_frame_impl.cc(6509)] content::RenderFrameImpl::RegisterMojoInterfaces
[20984:9408:1214/145534.447:ERROR:render_frame_impl.cc(6523)] content::RenderFrameImpl::OnHostZoomClientRequest
[22648:9172:1214/145534.902:ERROR:host_zoom_map_observer.cc(42)] content::HostZoomMapObserver::ReadyToCommitNavigation browser: https://news.ycombinator.com/ == 3.06939
[20984:9408:1214/145534.915:ERROR:render_frame_impl.cc(4861)] content::RenderFrameImpl::SendDidCommitProvisionalLoad  doing nothing



WORKED CORRECTLY:

[22648:9172:1214/145614.585:ERROR:host_zoom_map_observer.cc(42)] content::HostZoomMapObserver::ReadyToCommitNavigation browser: https://www.google.ca/_/chrome/newtab?rlz=1C1CHBF_enCA711CA711&espv=2&ie=UTF-8 == 0
[20524:7072:1214/145614.586:ERROR:render_frame_impl.cc(2679)] content::RenderFrameImpl::SetHostZoomLevel: https://www.google.ca/_/chrome/newtab?rlz=1C1CHBF_enCA711CA711&espv=2&ie=UTF-8 == 0
[20524:7072:1214/145614.617:ERROR:render_frame_impl.cc(4857)] content::RenderFrameImpl::SendDidCommitProvisionalLoad  first case: 0
[20524:7072:1214/145614.618:ERROR:render_frame_impl.cc(4869)] content::RenderFrameImpl::SendDidCommitProvisionalLoad  deleted
[22648:9172:1214/145615.136:ERROR:host_zoom_map_observer.cc(48)] content::HostZoomMapObserver::RenderFrameCreated
[22648:9172:1214/145615.184:ERROR:host_zoom_map_observer.cc(24)] ReadyToCommitNavigation, not main frame
[22648:9172:1214/145616.046:ERROR:host_zoom_map_observer.cc(48)] content::HostZoomMapObserver::RenderFrameCreated
[10236:2656:1214/145616.957:ERROR:render_frame_impl.cc(6509)] content::RenderFrameImpl::RegisterMojoInterfaces
[10236:2656:1214/145617.001:ERROR:render_frame_impl.cc(6523)] content::RenderFrameImpl::OnHostZoomClientRequest
[22648:9172:1214/145617.539:ERROR:host_zoom_map_observer.cc(48)] content::HostZoomMapObserver::RenderFrameCreated
[22580:22064:1214/145618.384:ERROR:render_frame_impl.cc(6509)] content::RenderFrameImpl::RegisterMojoInterfaces
[22580:22064:1214/145618.426:ERROR:render_frame_impl.cc(6523)] content::RenderFrameImpl::OnHostZoomClientRequest
[22648:9172:1214/145618.686:ERROR:host_zoom_map_observer.cc(42)] content::HostZoomMapObserver::ReadyToCommitNavigation browser: https://news.ycombinator.com/ == 3.06939
[22580:22064:1214/145618.687:ERROR:render_frame_impl.cc(2679)] content::RenderFrameImpl::SetHostZoomLevel: https://news.ycombinator.com/ == 3.06939
[22580:22064:1214/145618.701:ERROR:render_frame_impl.cc(4857)] content::RenderFrameImpl::SendDidCommitProvisionalLoad  first case: 3.06939
[22580:22064:1214/145618.704:ERROR:render_frame_impl.cc(4869)] content::RenderFrameImpl::SendDidCommitProvisionalLoad  deleted

Another bad side effect I've seen from the same cause is when following a link from a zoomed page to page without a saved zoom, the zoom doesn't get changed, so the new target is also zoomed unexpectedly.

I got interested in this based on Scott's chromium-mojo@ thread and am looking at it.

Aside from weighing in that this has been happening to me for a number of days and it's driving me insane, I will say that reproduction has been pretty unreliable. I normally work with chrome + a number of pinned application tabs open on ubuntu 16.04 and this seems to happen off and on throughout the day. I have noticed it from hacker news as the reporter suggested, as well as quite often on github which I normally keep zoomed to 125%. When a new tab is opened and the zoom isn't correct, both the zoom in the URL bar (magnifying glass with a +) and the zoom in the triple-dot settings menu report the correct saved zoom (e.g. 125% for github) but the page is not zoomed properly.

Sorry. There's a fix in progress, it should be landed soon.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8e24b7d133424fb221891601d6254b7df58188d5

commit 8e24b7d133424fb221891601d6254b7df58188d5
Author: blundell <blundell@chromium.org>
Date: Tue Dec 20 14:41:11 2016

Maintain HostZoom connection per-frame on browser side

HostZoomMapObserver is a per-WebContents class, while the HostZoom Mojo
connection is per-frame. Before this CL, HostZoomMapObserver was
maintaining one HostZoom connection and rebinding it every time a new
RenderFrame was created. This meant that HostZoomMapObserver was
continually losing connections to existing frames. This CL changes
HostZoomMapObserver to maintain one connection per-RenderFrame in a
map indexed by the corresponding RenderFrameHost.

BUG= 673065 
TEST=Visit news.ycombinator.com and increase the zoom level to 175%.
Click the top link. Hit back: news.ycombinator.com should still be
zoomed to 175%.

Review-Url: https://codereview.chromium.org/2581143002
Cr-Commit-Position: refs/heads/master@{#439800}

[modify] https://crrev.com/8e24b7d133424fb221891601d6254b7df58188d5/content/browser/host_zoom_map_observer.cc
[modify] https://crrev.com/8e24b7d133424fb221891601d6254b7df58188d5/content/browser/host_zoom_map_observer.h
[modify] https://crrev.com/8e24b7d133424fb221891601d6254b7df58188d5/content/browser/iframe_zoom_browsertest.cc
[add] https://crrev.com/8e24b7d133424fb221891601d6254b7df58188d5/content/test/data/page_with_iframe_and_link.html

[Auto-generated comment by a script] We noticed that this issue is targeted for M-56; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-56 label, otherwise remove Merge-TBD label. Thanks.

Likely too late to merge to 56, but will leave it for the TPM to decide :).

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a5ec18806970f9f7a4b2b1f5df55c49b40b2a9ed

commit a5ec18806970f9f7a4b2b1f5df55c49b40b2a9ed
Author: blundell <blundell@chromium.org>
Date: Tue Dec 20 15:54:34 2016

Revert of Maintain HostZoom connection per-frame on browser side (patchset #4 id:100001 of https://codereview.chromium.org/2581143002/ )

Reason for revert:
Seems to cause problems on Mac:

https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.webkit%2FWebKit_Mac10.10%2F28001%2F%2B%2Frecipes%2Fsteps%2Fwebkit_tests%2F0%2Fstdout

Original issue's description:
> Maintain HostZoom connection per-frame on browser side
>
> HostZoomMapObserver is a per-WebContents class, while the HostZoom Mojo
> connection is per-frame. Before this CL, HostZoomMapObserver was
> maintaining one HostZoom connection and rebinding it every time a new
> RenderFrame was created. This meant that HostZoomMapObserver was
> continually losing connections to existing frames. This CL changes
> HostZoomMapObserver to maintain one connection per-RenderFrame in a
> map indexed by the corresponding RenderFrameHost.
>
> BUG= 673065 
> TEST=Visit news.ycombinator.com and increase the zoom level to 175%.
> Click the top link. Hit back: news.ycombinator.com should still be
> zoomed to 175%.
>
> Committed: https://crrev.com/8e24b7d133424fb221891601d6254b7df58188d5
> Cr-Commit-Position: refs/heads/master@{#439800}

TBR=scottmg@chromium.org,wjmaclean@chromium.org,nick@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 673065 

Review-Url: https://codereview.chromium.org/2595603002
Cr-Commit-Position: refs/heads/master@{#439810}

[modify] https://crrev.com/a5ec18806970f9f7a4b2b1f5df55c49b40b2a9ed/content/browser/host_zoom_map_observer.cc
[modify] https://crrev.com/a5ec18806970f9f7a4b2b1f5df55c49b40b2a9ed/content/browser/host_zoom_map_observer.h
[modify] https://crrev.com/a5ec18806970f9f7a4b2b1f5df55c49b40b2a9ed/content/browser/iframe_zoom_browsertest.cc
[delete] https://crrev.com/2d77105b4666bf5b24de1f199e46e8164b241e00/content/test/data/page_with_iframe_and_link.html

I dug into the /fast/css/preserve-user-specified-zoom-level-on-reload.html test failure today and ended up confused:

(1) That test fails for me locally on ToT on Trusty Linux (release / non-component build to match the bot). The failure is identical with or without my patch: the "actual" image shows that the text isn't zoomed.

(2) I locally reverted scottmg's change (https://codereview.chromium.org/2374863002): the test still fails identically.

(3) On looking at the test, I don't understand why it is supposed to succeed in the first place. The zoomPageIn() calls made from JS end up calling here: https://cs.chromium.org/chromium/src/components/test_runner/event_sender.cc?q=event_sender.cc&sq=package:chromium&l=1806, which just calls setZoomIn() directly on Blink's WebViewImpl.cpp class, never updating the browser. When the page is reloaded, the browser sends a message that the host zoom level is the default, which results in //content/renderer setting WebViewImpl's zoom back to the default. This logical behavior was the same before and after scottmg or my changes and matches the failing test behavior that I'm seeing. What am I missing here?

I'm happy to keep working on this, but right now I'm blocked by the problems above. 

Your change meets the bar and is auto-approved for M56 (branch: 2924)

Given that the change has now been reverted on trunk, I'm going to remove these labels. Depending on when it relands, we'll see about requesting merge again.

It would be nice to know (i) when this test started failing, and (ii) why it's not failing on the bots. I wonder if it's racy (and if it once wasn't racy, but became so through some CL along the way).

Re: comment #20, I agree that the answers to those questions would be good (especially (ii)). As far as I can see though, there's nothing in the code that would indicate that this test *should* succeed (this is my point (3) above). I don't see that modifying zoom level only in the renderer followed by doing a reload would preserve that modified zoom level (since the browser resets zoom level on a reload).

I don't think the browser *is* supposed to reset zoom on a reload. I'm digging into this now, and will post my findings on the bug.

Re: #22, that was my original thought as well, which is why I reverted scottmg's CL locally. If there was a behavioral change in this regard, it was caused earlier than Scott's CL.

Notice also this comment: https://cs.chromium.org/chromium/src/content/renderer/render_frame_impl.cc?q=render_frame_impl.cc&sq=package:chromium&dr&l=4922.

So after spending some time digging through this, I'm thinking

* in recent history (last 1-2 years) I can't see why this test would pass
* yet it does, but seemingly only on the bots
  - so far I can only imagine this is a race, where the bot is grabbing pixels before the screen redraws the reloaded page, and similarly gets the layer tree dump prior to the zoom level being reset by the 
* the mechanisms that preserve a page zoom level on reload require browser interaction, and EventSender::zoomPageIn() (and related methods) bypass the browser completely

Here are some **very** interesting logs ... I added some printf statements in Loader::reload() and at a few points in RenderFrameImpl::didCommitProvisionalLoad() (see https://codereview.chromium.org/2596033002/ for details)

Here's what happens when you run the test by itself:

out/Debug/content_shell --run-layout-test --no-sandbox fast/css/preserve-user-specified-zoom-level-on-reload.html
[13264:13283:1221/182106.501882:806906456641:ERROR:entry.cc(167)] Entry::Deserialize: dictionary has no interface_provider_specs key
#READY
[13264:13283:1221/182106.523226:806906477914:ERROR:entry.cc(167)] Entry::Deserialize: dictionary has no interface_provider_specs key
[13264:13283:1221/182106.523639:806906478326:ERROR:entry.cc(167)] Entry::Deserialize: dictionary has no interface_provider_specs key
[13264:13283:1221/182106.527531:806906482218:ERROR:entry.cc(167)] Entry::Deserialize: dictionary has no interface_provider_specs key
[13264:13283:1221/182106.529685:806906484372:ERROR:entry.cc(167)] Entry::Deserialize: dictionary has no interface_provider_specs key
[13264:13283:1221/182106.530073:806906484759:ERROR:entry.cc(167)] Entry::Deserialize: dictionary has no interface_provider_specs key
[13264:13283:1221/182106.535898:806906490585:ERROR:entry.cc(167)] Entry::Deserialize: dictionary has no interface_provider_specs key
wjm: RFI::didCommitProvisionalLoad(0x31e5bb8f2020) zoom=0.000000
wjm: RFI::didCommitProvisionalLoad(0x31e5bb8f2020) erasing zoom level
wjm: Location::reload()
wjm: RFI::didCommitProvisionalLoad(0x31e5bb8f2020) zoom=0.000000
wjm: RFI::didCommitProvisionalLoad(0x31e5bb8f2020) erasing zoom level
Content-Type: text/plain
layer at (0,0) size 800x600
  LayoutView at (0,0) size 800x600
layer at (0,0) size 800x600
  LayoutBlockFlow {HTML} at (0,0) size 800x600
    LayoutBlockFlow {BODY} at (8,8) size 784x576
      LayoutBlockFlow {P} at (0,0) size 784x20
        LayoutText {#text} at (0,0) size 517x19
          text run at (0,0) width 517: "This test ensures that we preserve the user-specified zoom level of the page on reload."
#EOF
#EOF
wjm: RFI::didCommitProvisionalLoad(0x31e5bb8f2020) no zoom level
#EOF

***************
Now, here's what happens when you run

Tools/Scripts/run-webkit-tests --debug LayoutTests/fast/css/ --verbose --driver-logging

you get

[1080/1401] fast/css/preferred-stylesheet-reversed-order.html
 IN: /usr/local/google_ssd/projects/chromium/src/third_party/WebKit/LayoutTests/fast/css/preferred-stylesheet-reversed-order.html'--pixel-test\n
ERR: wjm: RFI::didCommitProvisionalLoad(0x16e8080f9020) no zoom level\n
OUT: Content-Type: text/plain\n
OUT: Preferred stylesheet where insertion order is tree order\n
OUT: \n
OUT: On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".\n
OUT: \n
OUT: \n
OUT: PASS getComputedStyle(t1).color is "rgb(0, 128, 0)"\n
OUT: PASS document.preferredStylesheetSet is "preferred"\n
OUT: PASS document.selectedStylesheetSet is "preferred"\n
OUT: PASS successfullyParsed is true\n
OUT: \n
OUT: TEST COMPLETE\n
OUT: This text should be green\n
OUT: #EOF\n
OUT: #EOF\n
ERR: wjm: RFI::didCommitProvisionalLoad(0x16e8080f9020) no zoom level\n
ERR: #EOF\n
[1081/1401] fast/css/preferred-stylesheet-reversed-order.html passed
[1081/1401] fast/css/preserve-user-specified-zoom-level-on-reload.html
 IN: /usr/local/google_ssd/projects/chromium/src/third_party/WebKit/LayoutTests/fast/css/preserve-user-specified-zoom-level-on-reload.html'--pixel-test'f8cd6f2846447238776daf95d9c741f9\n
ERR: wjm: RFI::didCommitProvisionalLoad(0x16e8080f9020) no zoom level\n
ERR: wjm: Location::reload()\n
ERR: wjm: RFI::didCommitProvisionalLoad(0x16e8080f9020) no zoom level\n
OUT: Content-Type: text/plain\n
OUT: layer at (0,0) size 800x600\n
OUT:   LayoutView at (0,0) size 800x600\n
OUT: layer at (0,0) size 800x600\n
OUT:   LayoutBlockFlow {HTML} at (0,0) size 800x600\n
OUT:     LayoutBlockFlow {BODY} at (23.88,23.88) size 752.25x528.36\n
OUT:       LayoutBlockFlow {P} at (0,0) size 752.25x171\n
OUT:         LayoutText {#text} at (0,1) size 708x169\n
OUT:           text run at (0,1) width 708: "This test ensures that we preserve the"\n
OUT:           text run at (0,58) width 704: "user-specified zoom level of the page"\n
OUT:           text run at (0,115) width 187: "on reload."\n
OUT: #EOF\n
OUT: \n
OUT: ActualHash: f8cd6f2846447238776daf95d9c741f9\n
OUT: \n
OUT: ExpectedHash: f8cd6f2846447238776daf95d9c741f9\n
OUT: #EOF\n
ERR: wjm: RFI::didCommitProvisionalLoad(0x16e8080f9020) no zoom level\n
ERR: #EOF\n
[1082/1401] fast/css/preserve-user-specified-zoom-level-on-reload.html passed
[1082/1401] fast/css/pseudo-active-display-none.html
 IN: /usr/local/google_ssd/projects/chromium/src/third_party/WebKit/LayoutTests/fast/css/pseudo-active-display-none.html'--pixel-test\n
ERR: wjm: RFI::didCommitProvisionalLoad(0x16e8080f9020) no zoom level\n
OUT: Content-Type: text/plain\n
OUT: This is a testharness.js-based test.\n
OUT: PASS window.eventSender is required for the test to run \n
OUT: PASS This test performs a check for active pseudo class to be applied for element when its display is set to none. \n
OUT: Harness: the test ran to completion.\n
OUT: \n
OUT: #EOF\n
OUT: #EOF\n
ERR: wjm: RFI::didCommitProvisionalLoad(0x16e8080f9020) no zoom level\n
ERR: #EOF\n

So the code that looks up the zoom level in the browser appears to not run when multiple tests run together, hence why this test seems to work reliably.

I should point out that the second output above is a section of the output from running ~1400 fast/css tests together, and the message

wjm: RFI::didCommitProvisionalLoad(0x31e5bb8f2020) zoom=......

is never seen once in the output.

Great find! With a local repro, I was able to figure out why the test passes on the bots with scottmg's change: The same WebContentsImpl gets reused across multiple tests, meaning that HostZoomMapObserver stomps on its |host_zoom_| connection. This in turn means that when the SetHostZoomLevel() call that it sends doesn't actually make it to the RenderFrameImpl being used in the preserve-user-zoom test, so RenderFrameImpl doesn't reset the zoom and hence the test succeeds. In other words, the test succeeds because of a bug :P.

This explanation doesn't indicate why the test would have been passing on the bots *before* scottmg's change. Indeed, when I ran the whole test suite locally with scottmg's change reverted, I see that the test still fails. So I still don't have an explanation for any discrepancy there.

Regardless, my suggestion would be to disable the test since it's broken, reland my CL, and open a separate bug about renderer-initiated zooms not being properly propagated to the browser. I have no idea whether renderer-initiated zooms can even occur in practice, or if this is basically a testing-only code path (in which case the test seems meaningless).

I agree, though with a slight modification.

We do actually preserve the zoom on reload if it was set from the browser side, so I would propose *deleting* the old layout test (which relies on a completely unrealistic, renderer-only mechanism), and *adding* a browser test that sets a zoom from the browser side (same pathways as the UI would use for a user), do the reload, and verify the new zoom level gets picked up. There may already be something like this, but if it doesn't exist we should add it.

I'll transfer this back to you (blundell@) if that's ok ...

SGTM.

Still able to reproduce the issue on windows 7 using chrome version 57.0.2969.0 and beta 56.0.2924.28.

blundell@Please update on this stable blocker issue.

Thanks,

Hi,

There are several CLs now out for review, of which the following two will need to land and be cherrypicked to solve this on M56:

- https://codereview.chromium.org/2612543002/
- https://codereview.chromium.org/2609003004/

 Issue 677928  has been merged into this issue.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/678cad15582afb68c0af5267e9753095b01989f2

commit 678cad15582afb68c0af5267e9753095b01989f2
Author: blundell <blundell@chromium.org>
Date: Wed Jan 04 20:14:57 2017

Add browsertest that zoom is preserved on reload

Zoom should be preserved on reload. However, this behavior is not currently
reliably covered by tests (there is a layout test that attempts to cover this
behavior, but it is broken and will shortly be removed, as per comment 28 on
 crbug.com/673065 ). This CL adds a proper test of this behavior.

BUG= 673065 

Review-Url: https://codereview.chromium.org/2608213003
Cr-Commit-Position: refs/heads/master@{#441449}

[rename] https://crrev.com/678cad15582afb68c0af5267e9753095b01989f2/content/browser/zoom_browsertest.cc
[modify] https://crrev.com/678cad15582afb68c0af5267e9753095b01989f2/content/test/BUILD.gn

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8872b06af376de3ebc7e96aec1fcbe68dae5d27a

commit 8872b06af376de3ebc7e96aec1fcbe68dae5d27a
Author: blundell <blundell@chromium.org>
Date: Thu Jan 05 10:35:36 2017

Remove fundamentally-broken layouttest for zoom on reload

The test being preserved is attempting to test that zoom is preserved on
reload. However, the mechanism used to do the zoom is renderer-side
rather than browser-side. This mechanism (a) doesn't reflect reality and
(b) means that the browser is not informed of the initial zoom, making
the test fundamentally broken. It is passing on the tree due to a
combination of other bugs (see the discussion on  crbug.com/673065  for
details).

This CL removes the test. It has been replaced by a browsertest that
exercises this functionality in a manner reflecting real-world usage.

BUG= 673065 

Review-Url: https://codereview.chromium.org/2612543002
Cr-Commit-Position: refs/heads/master@{#441617}

[delete] https://crrev.com/287e86e1c12c26ad850476f1410f2002b0521c8a/third_party/WebKit/LayoutTests/fast/css/preserve-user-specified-zoom-level-on-reload.html
[delete] https://crrev.com/287e86e1c12c26ad850476f1410f2002b0521c8a/third_party/WebKit/LayoutTests/platform/linux/fast/css/preserve-user-specified-zoom-level-on-reload-expected.png
[delete] https://crrev.com/287e86e1c12c26ad850476f1410f2002b0521c8a/third_party/WebKit/LayoutTests/platform/linux/fast/css/preserve-user-specified-zoom-level-on-reload-expected.txt
[delete] https://crrev.com/287e86e1c12c26ad850476f1410f2002b0521c8a/third_party/WebKit/LayoutTests/platform/mac/fast/css/preserve-user-specified-zoom-level-on-reload-expected.png
[delete] https://crrev.com/287e86e1c12c26ad850476f1410f2002b0521c8a/third_party/WebKit/LayoutTests/platform/mac/fast/css/preserve-user-specified-zoom-level-on-reload-expected.txt
[delete] https://crrev.com/287e86e1c12c26ad850476f1410f2002b0521c8a/third_party/WebKit/LayoutTests/platform/win/fast/css/preserve-user-specified-zoom-level-on-reload-expected.png
[delete] https://crrev.com/287e86e1c12c26ad850476f1410f2002b0521c8a/third_party/WebKit/LayoutTests/platform/win/fast/css/preserve-user-specified-zoom-level-on-reload-expected.txt

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e7aa3298381a03045a4a6dc6379990275dbfb247

commit e7aa3298381a03045a4a6dc6379990275dbfb247
Author: blundell <blundell@chromium.org>
Date: Thu Jan 05 12:38:55 2017

[Reland] Maintain HostZoom connection per-frame on browser side

This CL is a reland of https://codereview.chromium.org/2581143002.
That CL was reverted due to a failing layouttest. Investigation revealed
that the layouttest itself was busted, and that test has now been
removed (see the discussion on  crbug.com/673065  for details).

Original CL description:

HostZoomMapObserver is a per-WebContents class, while the HostZoom Mojo
connection is per-frame. Before this CL, HostZoomMapObserver was
maintaining one HostZoom connection and rebinding it every time a new
RenderFrame was created. This meant that HostZoomMapObserver was
continually losing connections to existing frames. This CL changes
HostZoomMapObserver to maintain one connection per-RenderFrame in a
map indexed by the corresponding RenderFrameHost.

BUG= 673065 
TEST=Visit news.ycombinator.com and increase the zoom level to 175%.
Click the top link. Hit back: news.ycombinator.com should still be
zoomed to 175%.

TBR=jochen

Review-Url: https://codereview.chromium.org/2609003004
Cr-Commit-Position: refs/heads/master@{#441629}

[modify] https://crrev.com/e7aa3298381a03045a4a6dc6379990275dbfb247/content/browser/host_zoom_map_observer.cc
[modify] https://crrev.com/e7aa3298381a03045a4a6dc6379990275dbfb247/content/browser/host_zoom_map_observer.h
[modify] https://crrev.com/e7aa3298381a03045a4a6dc6379990275dbfb247/content/browser/zoom_browsertest.cc
[add] https://crrev.com/e7aa3298381a03045a4a6dc6379990275dbfb247/content/test/data/page_with_iframe_and_link.html

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/dbe99b00fc1b41191214bb2c68c7f8b0b11487bd

commit dbe99b00fc1b41191214bb2c68c7f8b0b11487bd
Author: blundell <blundell@chromium.org>
Date: Thu Jan 05 17:15:23 2017

Remove dead ViewMsg_{Zoom, DidZoomUrl}

These messages and surrounding infra seem to be left over from prior
implementations of various pieces of zoom functionality (e.g., zooming
in plugins). The code being removed is never exercised:

- The browser-side entrypoint for sending ViewMsg_Zoom is
  RenderViewHost::Zoom(), which is never called.
- The renderer-side entrypoint for sending ViewMsg_DidZoomUrl is
  RenderViewImpl::zoomLevelChanged(), which is never called.

I noticed this while investigating  crbug.com/673065 .

BUG= 673065 

Review-Url: https://codereview.chromium.org/2611573004
Cr-Commit-Position: refs/heads/master@{#441679}

[modify] https://crrev.com/dbe99b00fc1b41191214bb2c68c7f8b0b11487bd/content/browser/renderer_host/render_view_host_impl.cc
[modify] https://crrev.com/dbe99b00fc1b41191214bb2c68c7f8b0b11487bd/content/browser/renderer_host/render_view_host_impl.h
[modify] https://crrev.com/dbe99b00fc1b41191214bb2c68c7f8b0b11487bd/content/common/view_messages.h
[modify] https://crrev.com/dbe99b00fc1b41191214bb2c68c7f8b0b11487bd/content/public/browser/render_view_host.h
[modify] https://crrev.com/dbe99b00fc1b41191214bb2c68c7f8b0b11487bd/content/renderer/render_view_impl.cc
[modify] https://crrev.com/dbe99b00fc1b41191214bb2c68c7f8b0b11487bd/content/renderer/render_view_impl.h

[Auto-generated comment by a script] We noticed that this issue is targeted for M-56; it appears the fix may have landed after branch point, meaning a merge might be required. Please confirm if a merge is required here - if so add Merge-Request-56 label, otherwise remove Merge-TBD label. Thanks.

Requesting merge for:

- https://codereview.chromium.org/2612543002 (assuming we care about keeping tests green on the stable branch :)
- https://codereview.chromium.org/2609003004 (the CL that fixes the bug)

Issue 663261 has been merged into this issue.

This is marked as fixed, but I'm still seeing it in the latest canary (57.0.2973.0).  Can you confirm that this is as expected?

This bug requires manual review: Reverts referenced in bugdroid comments after merge request.
Please contact the milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), gkihumba@(cros), bustamante@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

Re: comment #41, the reland (r441629) is not in 57.0.2973.0 (branched at 441616). Let me know if you still see the behavior in the *next* canary.

Works in 57.0.2975.0.  Thanks.

Approved for Merge into M56

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/af859c2e455484b3c877e6db55c20b4cd4a37113

commit af859c2e455484b3c877e6db55c20b4cd4a37113
Author: Colin Blundell <blundell@chromium.org>
Date: Tue Jan 10 12:31:55 2017

Remove fundamentally-broken layouttest for zoom on reload

The test being preserved is attempting to test that zoom is preserved on
reload. However, the mechanism used to do the zoom is renderer-side
rather than browser-side. This mechanism (a) doesn't reflect reality and
(b) means that the browser is not informed of the initial zoom, making
the test fundamentally broken. It is passing on the tree due to a
combination of other bugs (see the discussion on  crbug.com/673065  for
details).

This CL removes the test. It has been replaced by a browsertest that
exercises this functionality in a manner reflecting real-world usage.

BUG= 673065 

Review-Url: https://codereview.chromium.org/2612543002
Cr-Commit-Position: refs/heads/master@{#441617}
(cherry picked from commit 8872b06af376de3ebc7e96aec1fcbe68dae5d27a)

Review-Url: https://codereview.chromium.org/2625683002 .
Cr-Commit-Position: refs/branch-heads/2924@{#712}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[delete] https://crrev.com/85bd7cec4418fc7af0e10c206b28e9cdece43055/third_party/WebKit/LayoutTests/fast/css/preserve-user-specified-zoom-level-on-reload.html
[delete] https://crrev.com/85bd7cec4418fc7af0e10c206b28e9cdece43055/third_party/WebKit/LayoutTests/platform/linux/fast/css/preserve-user-specified-zoom-level-on-reload-expected.png
[delete] https://crrev.com/85bd7cec4418fc7af0e10c206b28e9cdece43055/third_party/WebKit/LayoutTests/platform/linux/fast/css/preserve-user-specified-zoom-level-on-reload-expected.txt
[delete] https://crrev.com/85bd7cec4418fc7af0e10c206b28e9cdece43055/third_party/WebKit/LayoutTests/platform/mac/fast/css/preserve-user-specified-zoom-level-on-reload-expected.png
[delete] https://crrev.com/85bd7cec4418fc7af0e10c206b28e9cdece43055/third_party/WebKit/LayoutTests/platform/mac/fast/css/preserve-user-specified-zoom-level-on-reload-expected.txt
[delete] https://crrev.com/85bd7cec4418fc7af0e10c206b28e9cdece43055/third_party/WebKit/LayoutTests/platform/win/fast/css/preserve-user-specified-zoom-level-on-reload-expected.png
[delete] https://crrev.com/85bd7cec4418fc7af0e10c206b28e9cdece43055/third_party/WebKit/LayoutTests/platform/win/fast/css/preserve-user-specified-zoom-level-on-reload-expected.txt

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/31fcef32237693e060e1d4286532febfd9627232

commit 31fcef32237693e060e1d4286532febfd9627232
Author: Colin Blundell <blundell@chromium.org>
Date: Tue Jan 10 12:42:55 2017

[Reland] Maintain HostZoom connection per-frame on browser side

This CL is a reland of https://codereview.chromium.org/2581143002.
That CL was reverted due to a failing layouttest. Investigation revealed
that the layouttest itself was busted, and that test has now been
removed (see the discussion on  crbug.com/673065  for details).

Original CL description:

HostZoomMapObserver is a per-WebContents class, while the HostZoom Mojo
connection is per-frame. Before this CL, HostZoomMapObserver was
maintaining one HostZoom connection and rebinding it every time a new
RenderFrame was created. This meant that HostZoomMapObserver was
continually losing connections to existing frames. This CL changes
HostZoomMapObserver to maintain one connection per-RenderFrame in a
map indexed by the corresponding RenderFrameHost.

BUG= 673065 
TEST=Visit news.ycombinator.com and increase the zoom level to 175%.
Click the top link. Hit back: news.ycombinator.com should still be
zoomed to 175%.

TBR=jochen

Review-Url: https://codereview.chromium.org/2609003004
Cr-Commit-Position: refs/heads/master@{#441629}
(cherry picked from commit e7aa3298381a03045a4a6dc6379990275dbfb247)

Review-Url: https://codereview.chromium.org/2619353005 .
Cr-Commit-Position: refs/branch-heads/2924@{#713}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[modify] https://crrev.com/31fcef32237693e060e1d4286532febfd9627232/content/browser/host_zoom_map_observer.cc
[modify] https://crrev.com/31fcef32237693e060e1d4286532febfd9627232/content/browser/host_zoom_map_observer.h

The relevant CLs are now merged. For the record: https://codereview.chromium.org/2609003004 added a browsertest on trunk, which did *not* get merged to 56 (there were conflicts in the test file and the easiest action was to simply not merge in the new browsertest).

Tested the issue on Ubuntu 14.04,Win 7 and Mac OS 10.12.2 using chrome latest Beta M56-56.0.2924.59 by following steps mentioned in the original comment. Observed that Press back arrow page zoom level is 175% as expected. Hence adding TE-Verified label.Please find the screen shot for reference.

Thank you!

Deleted: 673065.ogv 4.3 MB

	673065.ogv 4.3 MB View Download