Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1953503005:
RenderView needs to know zoom level for loading pages. (Closed)
|
Created:
4 years, 7 months ago by wjmaclean Modified:
4 years, 7 months ago Reviewers:
Charlie Reis CC:
chromium-reviews, mlamouri+watch-content_chromium.org, creis+watch_chromium.org, nasko+codewatch_chromium.org, jam, darin-cc_chromium.org, mkwst+moarreviews-renderer_chromium.org, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRenderView needs to know zoom level for loading pages.
Since r390755 (https://codereview.chromium.org/1804023002),
RenderViewImpl keeps track of zoom level for pages so it can be applied
when subframes load.
This CL fixes a bug where RenderFrameImpl directly sets the zoom level
on the WebView for loading pages, without going through RenderViewImpl.
This can lead to incorrect zoom levels when redirects occur, or a page
is loaded with middle-click. The effect is only seen when the page
being loaded contains subframes.
This CL forces the zoom change to go via RenderViewImpl. There is a
slight behaviour change in this, as this will trigger
RenderViewObserver::OnZoomLevelChanged() notifications for a loading
page, where it did not before. However, at present the only consumer
of this notification is WebViewPlugin, and it is not expected the
change will cause problems in this case.
BUG=608798, 609213
Committed: https://crrev.com/83f0cc71189113188073aaceee1e5e3dc46f8f08
Cr-Commit-Position: refs/heads/master@{#392684}
Patch Set 1 #Patch Set 2 : Add test. #Patch Set 3 : Bug/test not relevant to Android, don't try to run test there. #
Total comments: 6
Patch Set 4 : Remove stale comment. #Messages
Total messages: 31 (15 generated)
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1953503005/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1953503005/1
Initial patch to run on the trybots; test coming Monday.
Description was changed from ========== RenderView needs to know zoom level for loading pages. Since r390755 (https://codereview.chromium.org/1804023002), RenderViewImpl keeps track of zoom level for pages so it can be applied when subframes load. This CL fixes a bug where RenderFrameImpl directly sets the zoom level on the WebView for loading pages, without going through WebView. This can lead to incorrect zoom levels when redirects occur, or a page is loaded with middle-click. This CL forces the zoom change to go via RenderViewImpl. There is a slight behaviour change in this, as this will trigger RenderViewObserver::OnZoomLevelChanged() notifications for a loading page, where it did not before. However, at present the only consumer of this notification is WebViewPlugin, and it is not expected the change will cause problems in this case. BUG=608798, 609213 ========== to ========== RenderView needs to know zoom level for loading pages. Since r390755 (https://codereview.chromium.org/1804023002), RenderViewImpl keeps track of zoom level for pages so it can be applied when subframes load. This CL fixes a bug where RenderFrameImpl directly sets the zoom level on the WebView for loading pages, without going through RenderViewImpl. This can lead to incorrect zoom levels when redirects occur, or a page is loaded with middle-click. This CL forces the zoom change to go via RenderViewImpl. There is a slight behaviour change in this, as this will trigger RenderViewObserver::OnZoomLevelChanged() notifications for a loading page, where it did not before. However, at present the only consumer of this notification is WebViewPlugin, and it is not expected the change will cause problems in this case. BUG=608798, 609213 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1953503005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1953503005/20001
wjmaclean@chromium.org changed reviewers: + creis@chromium.org
Description was changed from ========== RenderView needs to know zoom level for loading pages. Since r390755 (https://codereview.chromium.org/1804023002), RenderViewImpl keeps track of zoom level for pages so it can be applied when subframes load. This CL fixes a bug where RenderFrameImpl directly sets the zoom level on the WebView for loading pages, without going through RenderViewImpl. This can lead to incorrect zoom levels when redirects occur, or a page is loaded with middle-click. This CL forces the zoom change to go via RenderViewImpl. There is a slight behaviour change in this, as this will trigger RenderViewObserver::OnZoomLevelChanged() notifications for a loading page, where it did not before. However, at present the only consumer of this notification is WebViewPlugin, and it is not expected the change will cause problems in this case. BUG=608798, 609213 ========== to ========== RenderView needs to know zoom level for loading pages. Since r390755 (https://codereview.chromium.org/1804023002), RenderViewImpl keeps track of zoom level for pages so it can be applied when subframes load. This CL fixes a bug where RenderFrameImpl directly sets the zoom level on the WebView for loading pages, without going through RenderViewImpl. This can lead to incorrect zoom levels when redirects occur, or a page is loaded with middle-click. The effect is only seen when the page being loaded contains subframes. This CL forces the zoom change to go via RenderViewImpl. There is a slight behaviour change in this, as this will trigger RenderViewObserver::OnZoomLevelChanged() notifications for a loading page, where it did not before. However, at present the only consumer of this notification is WebViewPlugin, and it is not expected the change will cause problems in this case. BUG=608798, 609213 ==========
Description was changed from ========== RenderView needs to know zoom level for loading pages. Since r390755 (https://codereview.chromium.org/1804023002), RenderViewImpl keeps track of zoom level for pages so it can be applied when subframes load. This CL fixes a bug where RenderFrameImpl directly sets the zoom level on the WebView for loading pages, without going through RenderViewImpl. This can lead to incorrect zoom levels when redirects occur, or a page is loaded with middle-click. The effect is only seen when the page being loaded contains subframes. This CL forces the zoom change to go via RenderViewImpl. There is a slight behaviour change in this, as this will trigger RenderViewObserver::OnZoomLevelChanged() notifications for a loading page, where it did not before. However, at present the only consumer of this notification is WebViewPlugin, and it is not expected the change will cause problems in this case. BUG=608798, 609213 ========== to ========== RenderView needs to know zoom level for loading pages. Since r390755 (https://codereview.chromium.org/1804023002), RenderViewImpl keeps track of zoom level for pages so it can be applied when subframes load. This CL fixes a bug where RenderFrameImpl directly sets the zoom level on the WebView for loading pages, without going through RenderViewImpl. This can lead to incorrect zoom levels when redirects occur, or a page is loaded with middle-click. The effect is only seen when the page being loaded contains subframes. This CL forces the zoom change to go via RenderViewImpl. There is a slight behaviour change in this, as this will trigger RenderViewObserver::OnZoomLevelChanged() notifications for a loading page, where it did not before. However, at present the only consumer of this notification is WebViewPlugin, and it is not expected the change will cause problems in this case. BUG=608798, 609213 ==========
creis@ - one liner (not including the new test), please take a look?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1953503005/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1953503005/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks for fixing this. There are more calls to SetZoomLevel in RenderFrameImpl than just that one. Are they problematic as well? https://codereview.chromium.org/1953503005/diff/40001/content/browser/iframe_... File content/browser/iframe_zoom_browsertest.cc (right): https://codereview.chromium.org/1953503005/diff/40001/content/browser/iframe_... content/browser/iframe_zoom_browsertest.cc:455: // TODO(wjmaclean): Need more reliable method of loading the redirected page. What do you mean by more reliable? Is this test going to be flaky? https://codereview.chromium.org/1953503005/diff/40001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1953503005/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:4487: render_view_->webview()->setZoomLevel(0); What about this one? https://codereview.chromium.org/1953503005/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:4570: render_view_->webview()->setZoomLevel(render_view_->page_zoom_level()); Is this one right?
Stale comment removed (the test should be stable). See other answers to your questions below. https://codereview.chromium.org/1953503005/diff/40001/content/browser/iframe_... File content/browser/iframe_zoom_browsertest.cc (right): https://codereview.chromium.org/1953503005/diff/40001/content/browser/iframe_... content/browser/iframe_zoom_browsertest.cc:455: // TODO(wjmaclean): Need more reliable method of loading the redirected page. On 2016/05/10 17:15:49, Charlie Reis wrote: > What do you mean by more reliable? Is this test going to be flaky? Ooops, no ... stale comment. It used to be flaky, and now it should be fine (I originally was using a standalone page to force the redirect, and this *was* flaky with NavigateToURLBlockUntilNavigationsComplete(), but then I switched to using /client-redirect --- which seems to be the preferred method for redirect tests --- and the flake seems to have gone away). https://codereview.chromium.org/1953503005/diff/40001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1953503005/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:4487: render_view_->webview()->setZoomLevel(0); On 2016/05/10 17:15:49, Charlie Reis wrote: > What about this one? This one is right, since presumably the plugin doc can't have any subframes. I left it as a direct call to webview()->setZoomLevel() to avoid changing notifications to any RVOs, though it *likely* would be safe to convert this call to being directly on the RenderView (perhaps in a separate CL, so that if it breaks anything we can revert it without losing the fix this CL provides). https://codereview.chromium.org/1953503005/diff/40001/content/renderer/render... content/renderer/render_frame_impl.cc:4570: render_view_->webview()->setZoomLevel(render_view_->page_zoom_level()); On 2016/05/10 17:15:49, Charlie Reis wrote: > Is this one right? This is right since it is using the page_zoom_level that the RenderView already knows about. I left this as a direct call to webview()->setZoomLevel() to avoid any changes in notifications sent to RVOs. (Though it might be safe to try converting it, perhaps in a separate CLl)
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1953503005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1953503005/60001
[+site-isolation-reviews] Thanks for clarifying. It's subtle enough that you might want a comment about why this case differs, but no need if you plan to change the other cases anyway. LGTM.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by wjmaclean@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1953503005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1953503005/60001
Message was sent while issue was closed.
Description was changed from ========== RenderView needs to know zoom level for loading pages. Since r390755 (https://codereview.chromium.org/1804023002), RenderViewImpl keeps track of zoom level for pages so it can be applied when subframes load. This CL fixes a bug where RenderFrameImpl directly sets the zoom level on the WebView for loading pages, without going through RenderViewImpl. This can lead to incorrect zoom levels when redirects occur, or a page is loaded with middle-click. The effect is only seen when the page being loaded contains subframes. This CL forces the zoom change to go via RenderViewImpl. There is a slight behaviour change in this, as this will trigger RenderViewObserver::OnZoomLevelChanged() notifications for a loading page, where it did not before. However, at present the only consumer of this notification is WebViewPlugin, and it is not expected the change will cause problems in this case. BUG=608798, 609213 ========== to ========== RenderView needs to know zoom level for loading pages. Since r390755 (https://codereview.chromium.org/1804023002), RenderViewImpl keeps track of zoom level for pages so it can be applied when subframes load. This CL fixes a bug where RenderFrameImpl directly sets the zoom level on the WebView for loading pages, without going through RenderViewImpl. This can lead to incorrect zoom levels when redirects occur, or a page is loaded with middle-click. The effect is only seen when the page being loaded contains subframes. This CL forces the zoom change to go via RenderViewImpl. There is a slight behaviour change in this, as this will trigger RenderViewObserver::OnZoomLevelChanged() notifications for a loading page, where it did not before. However, at present the only consumer of this notification is WebViewPlugin, and it is not expected the change will cause problems in this case. BUG=608798, 609213 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== RenderView needs to know zoom level for loading pages. Since r390755 (https://codereview.chromium.org/1804023002), RenderViewImpl keeps track of zoom level for pages so it can be applied when subframes load. This CL fixes a bug where RenderFrameImpl directly sets the zoom level on the WebView for loading pages, without going through RenderViewImpl. This can lead to incorrect zoom levels when redirects occur, or a page is loaded with middle-click. The effect is only seen when the page being loaded contains subframes. This CL forces the zoom change to go via RenderViewImpl. There is a slight behaviour change in this, as this will trigger RenderViewObserver::OnZoomLevelChanged() notifications for a loading page, where it did not before. However, at present the only consumer of this notification is WebViewPlugin, and it is not expected the change will cause problems in this case. BUG=608798, 609213 ========== to ========== RenderView needs to know zoom level for loading pages. Since r390755 (https://codereview.chromium.org/1804023002), RenderViewImpl keeps track of zoom level for pages so it can be applied when subframes load. This CL fixes a bug where RenderFrameImpl directly sets the zoom level on the WebView for loading pages, without going through RenderViewImpl. This can lead to incorrect zoom levels when redirects occur, or a page is loaded with middle-click. The effect is only seen when the page being loaded contains subframes. This CL forces the zoom change to go via RenderViewImpl. There is a slight behaviour change in this, as this will trigger RenderViewObserver::OnZoomLevelChanged() notifications for a loading page, where it did not before. However, at present the only consumer of this notification is WebViewPlugin, and it is not expected the change will cause problems in this case. BUG=608798, 609213 Committed: https://crrev.com/83f0cc71189113188073aaceee1e5e3dc46f8f08 Cr-Commit-Position: refs/heads/master@{#392684} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/83f0cc71189113188073aaceee1e5e3dc46f8f08 Cr-Commit-Position: refs/heads/master@{#392684} |
