Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2608213003:
Add browsertest that zoom is preserved on reload (Closed)
DescriptionAdd 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
Committed: https://crrev.com/678cad15582afb68c0af5267e9753095b01989f2
Cr-Commit-Position: refs/heads/master@{#441449}
Patch Set 1 #
Total comments: 2
Dependent Patchsets: Messages
Total messages: 23 (11 generated)
The CQ bit was checked by blundell@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Add browsertest that zoom is preserved on reload Zoom should be preserved on reload. However, this behavior is not currently covered by tests. This CL adds such a test. BUG=673065 ========== to ========== 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 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
blundell@chromium.org changed reviewers: + wjmaclean@chromium.org
Going to send out the removal of the layouttest separately in case the new test has to be reverted due to flakiness or whatever.
Just one question, otherwise it looks fine. https://codereview.chromium.org/2608213003/diff/1/content/browser/zoom_browse... File content/browser/zoom_browsertest.cc (right): https://codereview.chromium.org/2608213003/diff/1/content/browser/zoom_browse... content/browser/zoom_browsertest.cc:271: EXPECT_NEAR( Do we need to watch for any resize (or similar) events to make sure the previous zoom level is properly re-applied once everything settles?
https://codereview.chromium.org/2608213003/diff/1/content/browser/zoom_browse... File content/browser/zoom_browsertest.cc (right): https://codereview.chromium.org/2608213003/diff/1/content/browser/zoom_browse... content/browser/zoom_browsertest.cc:271: EXPECT_NEAR( On 2017/01/03 16:30:30, wjmaclean wrote: > Do we need to watch for any resize (or similar) events to make sure the previous > zoom level is properly re-applied once everything settles? That's an interesting question. Do you know if a resize event fires in this case, i.e., does the page load at default zoom and then have its zoom changed? If so, I would have expected that the way I've written the test here would fail, since I'm not waiting for any such event before checking that the page is zoomed.
On 2017/01/03 16:36:57, blundell wrote: > https://codereview.chromium.org/2608213003/diff/1/content/browser/zoom_browse... > File content/browser/zoom_browsertest.cc (right): > > https://codereview.chromium.org/2608213003/diff/1/content/browser/zoom_browse... > content/browser/zoom_browsertest.cc:271: EXPECT_NEAR( > On 2017/01/03 16:30:30, wjmaclean wrote: > > Do we need to watch for any resize (or similar) events to make sure the > previous > > zoom level is properly re-applied once everything settles? > > That's an interesting question. Do you know if a resize event fires in this > case, i.e., does the page load at default zoom and then have its zoom changed? > If so, I would have expected that the way I've written the test here would fail, > since I'm not waiting for any such event before checking that the page is > zoomed. I'm not sure how it works off-hand, but it would be good to verify what the current mechanism is so we don't end up with a flakey test (i.e. maybe your test is passing locally, but could suffer timing issues on some bots?).
On 2017/01/03 16:41:12, wjmaclean wrote: > On 2017/01/03 16:36:57, blundell wrote: > > > https://codereview.chromium.org/2608213003/diff/1/content/browser/zoom_browse... > > File content/browser/zoom_browsertest.cc (right): > > > > > https://codereview.chromium.org/2608213003/diff/1/content/browser/zoom_browse... > > content/browser/zoom_browsertest.cc:271: EXPECT_NEAR( > > On 2017/01/03 16:30:30, wjmaclean wrote: > > > Do we need to watch for any resize (or similar) events to make sure the > > previous > > > zoom level is properly re-applied once everything settles? > > > > That's an interesting question. Do you know if a resize event fires in this > > case, i.e., does the page load at default zoom and then have its zoom changed? > > If so, I would have expected that the way I've written the test here would > fail, > > since I'm not waiting for any such event before checking that the page is > > zoomed. > > I'm not sure how it works off-hand, but it would be good to verify what the > current mechanism is so we don't end up with a flakey test (i.e. maybe your test > is passing locally, but could suffer timing issues on some bots?). I spent some time investigating this. On a reload the zoom level for the main frame does not get reset in the renderer, so Blink just keeps its previously-set zoom level (this is true both in the test and in Chrome AFAICT). There also does not appear to be any resize event that occurs after the reload starts. It's not totally conclusive, but I dug about as deep as I could.
> I spent some time investigating this. On a reload the zoom level for the main > frame does not get reset in the renderer, so Blink just keeps its previously-set > zoom level (this is true both in the test and in Chrome AFAICT). There also does > not appear to be any resize event that occurs after the reload starts. It's not > totally conclusive, but I dug about as deep as I could. Interesting ... in a way then that makes it non-obvious that the test would catch a regression in the preserving of the scale factor after reload: for example, if a page reloaded at the same scale, then re-scaled (presumably causing a resize event), this test might not catch it. But then, just like it's harder to prove something *doesn't* exist, there might not be any reasonable test that would catch such a regression. But this test could catch some imaginable regressions, so I think it's worth landing it. LGTM
On 2017/01/04 18:26:02, wjmaclean wrote: > > I spent some time investigating this. On a reload the zoom level for the main > > frame does not get reset in the renderer, so Blink just keeps its > previously-set > > zoom level (this is true both in the test and in Chrome AFAICT). There also > does > > not appear to be any resize event that occurs after the reload starts. It's > not > > totally conclusive, but I dug about as deep as I could. > > Interesting ... in a way then that makes it non-obvious that the test would > catch a regression in the preserving of the scale factor after reload: for > example, if a page reloaded at the same scale, then re-scaled (presumably > causing a resize event), this test might not catch it. But then, just like it's > harder to prove something *doesn't* exist, there might not be any reasonable > test that would catch such a regression. > > But this test could catch some imaginable regressions, so I think it's worth > landing it. > > LGTM Agreed that this finding decreases the utility of this test, but that it still has non-zero utility.
blundell@chromium.org changed reviewers: + jochen@chromium.org
+jochen@ for //content OWNERS
lgtm
The CQ bit was checked by blundell@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1483556829104350, "parent_rev":
"29c1b7da08c5a12a98e9031c1f8d188cbd006a5b", "commit_rev":
"a1604b5a53bc15dbf90f322ace7dead54564ef1e"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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 Committed: https://crrev.com/678cad15582afb68c0af5267e9753095b01989f2 Cr-Commit-Position: refs/heads/master@{#441449} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/678cad15582afb68c0af5267e9753095b01989f2 Cr-Commit-Position: refs/heads/master@{#441449} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
