Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1804023002:
Fix page zoom to be frame-centric for out-of-process frames. (Closed)
|
Created:
4 years, 9 months ago by wjmaclean Modified:
4 years, 7 months ago CC:
blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, dglazkov+blink, jam, mkwst+moarreviews-renderer_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_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. |
DescriptionFix page zoom to be frame-centric for out-of-process frames.
This CL modifies the current methods for setting page zoom from the
browser to make them work properly with out-of-process frames.
HostZoomMapImpl now routes all zoom-level changes through the
WebContents as the latter knows about all the frames associated with
the page, regardless of which process/view they are in.
This CL also modifies how default page zoom is applied, by
having HostZoomMapImpl push the changes through the WebContents
instead of propagating them via the RendererPrefs. This should
make the code around default zoom levels easier to understand
as well.
It also avoids changing zoom levels on frames/views embedded in
a WebContents with a custom (non-default) zoom level.
BUG=528407
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/64951906c6fb59b33a63313f98819bbafe9eebe2
Cr-Commit-Position: refs/heads/master@{#390755}
Patch Set 1 #Patch Set 2 : Fix some tests. #Patch Set 3 : Convert temporary zoom to be frame-centric as well. #Patch Set 4 : Rebase to master@{#383721} #Patch Set 5 : Early out in UpdateZoomIfNecessary when no committed navigation entry. #Patch Set 6 : Fix support for default page zoom level. #Patch Set 7 : Send zoom change notifications for updates triggered by default page zoom changes. #
Total comments: 1
Patch Set 8 : Don't use WeakPtr for PostTask on UI thread. #
Total comments: 46
Patch Set 9 : Fix test. #Patch Set 10 : Address alexmos@ comments, run two experiments. #
Total comments: 6
Patch Set 11 : Revert one experiment. #Patch Set 12 : Convert to use PageMsg instead of FrameMsg. #
Total comments: 34
Patch Set 13 : Address alexmos@ comments #
Total comments: 4
Patch Set 14 : Rebase to master@{#386187}. #
Total comments: 16
Patch Set 15 : Address comments, fix test. #Patch Set 16 : Fix test compile. #
Total comments: 2
Patch Set 17 : Trying 'git add' this time ... #Patch Set 18 : Modify tests to accommodate window borders on other platforms. #Patch Set 19 : Revise tests to use constancy of innerWidth for fixed-size iframes. #
Total comments: 18
Patch Set 20 : Add tests, address comments. #
Total comments: 19
Patch Set 21 : Add navigation observer to test. #Patch Set 22 : Address comments, exclude part of test since not supported on Android. #
Total comments: 21
Patch Set 23 : Address comments. #
Total comments: 6
Patch Set 24 : Insert bug numbers before landing. #Patch Set 25 : Remove 'anonymous'. #
Total comments: 1
Messages
Total messages: 165 (68 generated)
Description was changed from ========== Fix page zoom to be frame-centric for out-of-process frames. This CL modifies the current methods for setting page zoom from the browser to make them work properly with out-of-process frames. HostZoomMapImpl now attempts to route zoom-level changes through the WebContents as the latter knows about all the frames associated with the page, regardless of which process/view they are in. BUG=528407 ========== to ========== ** This version has been uploaded to try on the bots. Fix page zoom to be frame-centric for out-of-process frames. This CL modifies the current methods for setting page zoom from the browser to make them work properly with out-of-process frames. HostZoomMapImpl now attempts to route zoom-level changes through the WebContents as the latter knows about all the frames associated with the page, regardless of which process/view they are in. BUG=528407 ==========
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/1804023002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804023002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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/1804023002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804023002/20001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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/1804023002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804023002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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/1804023002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804023002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win8_chromium_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...) win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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/1804023002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804023002/100001
Description was changed from ========== ** This version has been uploaded to try on the bots. Fix page zoom to be frame-centric for out-of-process frames. This CL modifies the current methods for setting page zoom from the browser to make them work properly with out-of-process frames. HostZoomMapImpl now attempts to route zoom-level changes through the WebContents as the latter knows about all the frames associated with the page, regardless of which process/view they are in. BUG=528407 ========== to ========== ** This version has been uploaded to try on the bots. Fix page zoom to be frame-centric for out-of-process frames. This CL modifies the current methods for setting page zoom from the browser to make them work properly with out-of-process frames. HostZoomMapImpl now routes all zoom-level changes through the WebContents as the latter knows about all the frames associated with the page, regardless of which process/view they are in. This CL also modifies how default page zoom is applied, by having HostZoomMapImpl push the changes through the WebContents instead of propagating them via the RendererPrefs. This should make the code around default zoom levels easier to understand as well. BUG=528407 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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/1804023002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804023002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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/1804023002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804023002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Description was changed from ========== ** This version has been uploaded to try on the bots. Fix page zoom to be frame-centric for out-of-process frames. This CL modifies the current methods for setting page zoom from the browser to make them work properly with out-of-process frames. HostZoomMapImpl now routes all zoom-level changes through the WebContents as the latter knows about all the frames associated with the page, regardless of which process/view they are in. This CL also modifies how default page zoom is applied, by having HostZoomMapImpl push the changes through the WebContents instead of propagating them via the RendererPrefs. This should make the code around default zoom levels easier to understand as well. BUG=528407 ========== to ========== Fix page zoom to be frame-centric for out-of-process frames. This CL modifies the current methods for setting page zoom from the browser to make them work properly with out-of-process frames. HostZoomMapImpl now routes all zoom-level changes through the WebContents as the latter knows about all the frames associated with the page, regardless of which process/view they are in. This CL also modifies how default page zoom is applied, by having HostZoomMapImpl push the changes through the WebContents instead of propagating them via the RendererPrefs. This should make the code around default zoom levels easier to understand as well. It also avoids changing zoom levels on frames/views embedded in a WebContents with a custom (non-default) zoom level. BUG=528407 ==========
wjmaclean@chromium.org changed reviewers: + creis@chromium.org
creis@ - could you take an initial look at this? I'm still trying to understand why NavigateToURL() is failing for two tests on Win/Mac/Android, but the CL is now pretty mature, and I'd appreciate your feedback on the approach.
creis@chromium.org changed reviewers: + nick@chromium.org
I'm happy to see this CL! I'm going to be a bit slow for the next several days, so I'll ask Nick to take a look at it for me.
nick@chromium.org changed reviewers: + alexmos@chromium.org - creis@chromium.org
Alex and I will review this.
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/1804023002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804023002/160001
wjmaclean@chromium.org changed reviewers: + kenrb@chromium.org, thestig@chromium.org
+thestig@ for renderer_preferences_util.cc +kenrb@ for *_messages.h
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks James, some initial comments below. Sorry for the delay -- I had to ramp up a bit on how zoom works today (and there are a few more questions on that below). https://codereview.chromium.org/1804023002/diff/120001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1804023002/diff/120001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:878: if (!entry) So this means that we ignore zoom changes when there's no committed entry? https://codereview.chromium.org/1804023002/diff/140001/content/browser/host_z... File content/browser/host_zoom_map_impl.cc (right): https://codereview.chromium.org/1804023002/diff/140001/content/browser/host_z... content/browser/host_zoom_map_impl.cc:287: // First, remove all entries that match the new default zoom level. Does this need to be done for scheme_host_zoom_levels_ too? (Side question: when are those used instead of host_zoom_levels_?) https://codereview.chromium.org/1804023002/diff/140001/content/browser/host_z... content/browser/host_zoom_map_impl.cc:306: // is different than is stored in the map. nit: s/than is stored/than the one stored/ https://codereview.chromium.org/1804023002/diff/140001/content/browser/host_z... content/browser/host_zoom_map_impl.cc:319: if (GetForWebContents(web_contents) == this && uses_default_zoom) { Help me understand: why do we want to filter this by "GetForWebContents(web_contents) == this"? Is this because HostZoomMap is per-profile and GetAllWebContents() isn't? A comment about this would be nice. https://codereview.chromium.org/1804023002/diff/140001/content/browser/host_z... content/browser/host_zoom_map_impl.cc:323: web_contents->GetController().GetLastCommittedEntry(); It seems |entry| is already set to last committed entry above, any reason to redo it here? https://codereview.chromium.org/1804023002/diff/140001/content/browser/host_z... content/browser/host_zoom_map_impl.cc:332: zoom_level_changed_callbacks_.Notify(change); I didn't see this notification used previously for default zoom level changes. Did I miss it, or is this a new thing you're adding? (and if so, why is it necessary?) Also, it seems weird that change.mode is set to scheme/host whereas this is changing default zoom. https://codereview.chromium.org/1804023002/diff/140001/content/browser/host_z... content/browser/host_zoom_map_impl.cc:532: int routing_id = web_contents->GetRenderViewHost()->GetRoutingID(); nit: render_view_id or view_routing_id might be a bit more readable. https://codereview.chromium.org/1804023002/diff/140001/content/browser/host_z... File content/browser/host_zoom_map_impl_browsertest.cc (right): https://codereview.chromium.org/1804023002/diff/140001/content/browser/host_z... content/browser/host_zoom_map_impl_browsertest.cc:62: EXPECT_TRUE(NavigateToURL(shell(), url)); It seems RunTestForURL will now redundantly navigate to this url again. https://codereview.chromium.org/1804023002/diff/140001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1804023002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:863: SendToAllFrames(new FrameMsg_SetTemporaryZoomLevel(MSG_ROUTING_NONE, level, Not being familiar with zoom, can you please explain when temporary zoom levels are used? Just looking at the code, it seemed this is something used when the top document is a plugin (so that plugin-specific zoom isn't saved), but for that case I don't understand why SendToAllFrames is necessary, since plugins shouldn't have frames. https://codereview.chromium.org/1804023002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:870: SendToAllFrames( I'm curious why we can't use SendPageMessage instead, which might reduce the redundancy in page-level zoom setting on the renderer side? Or is there some work that needs to be done on each frame (probably LocalFrame::setPageZoomFactor), and this just makes it easy to go through all the frames? https://codereview.chromium.org/1804023002/diff/140001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1804023002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:227: void SetTemporaryZoomLevel(double level, bool is_temporary); nit: comments explaining what the new functions are for? https://codereview.chromium.org/1804023002/diff/140001/content/common/frame_m... File content/common/frame_messages.h (right): https://codereview.chromium.org/1804023002/diff/140001/content/common/frame_m... content/common/frame_messages.h:793: // Instructs the frame to set its zoom level to match the owning WebContents. The comments for these two messages are the same - might want to explain what the difference is. https://codereview.chromium.org/1804023002/diff/140001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1804023002/diff/140001/content/renderer/rende... content/renderer/render_frame_impl.cc:1436: IPC_MESSAGE_HANDLER(FrameMsg_SetZoomLevelFromWebContents, "FromWebContents" sounded a bit weird in the name. It seems this is about replicating page zoom level, so would something like FrameMsg_SetPageZoomLevel be better? https://codereview.chromium.org/1804023002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1804023002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3031: double WebViewImpl::setZoomLevelForFrame(LocalFrame* frame, double zoomLevel) Why is the LocalFrame version of the function needed? Why not just have one version that takes a WebLocalFrame and convert to LocalFrame in its body? setZoomLevel() can invoke it with mainFrameImpl() instead of mainFrameImpl()->frame(). https://codereview.chromium.org/1804023002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3052: frame->setPageZoomFactor(zoomFactor); This calls LocalFrame::setPageAndTextZoomFactors, which also updates the zoom parameters for all local child frames: https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit.... Isn't this redundant, since with one IPC per frame, WebViewImpl::setZoomLevelForFrame will also be called separately for those child frames? https://codereview.chromium.org/1804023002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3058: double WebViewImpl::setZoomLevel(double zoomLevel) There seem to be some existing renderer-initiated call sites for this, such as setCompositorDeviceScaleFactorOverride from devtools. Would those need to separately replicate the zoom update to any other processes for the FrameTree? https://codereview.chromium.org/1804023002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3065: m_zoomLevel = zoomLevel; Is it necessary to have this duplicated here in addition to what's in setZoomLevelForFrame? I also don't see any callers caring about the return value. https://codereview.chromium.org/1804023002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3069: if (mainFrame()->isWebRemoteFrame()) How much would break if we ASSERT that mainFrame() is a local frame here, rather than leaving the escape hatch in place? https://codereview.chromium.org/1804023002/diff/140001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebView.h (right): https://codereview.chromium.org/1804023002/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/web/WebView.h:208: virtual double setZoomLevelForFrame(WebLocalFrame*, double zoomLevel) = 0; nit: comment
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/1804023002/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804023002/180001
Hi Alex, I've updated based on your comments ... I'm also including two experiments (sending notifications for HOST instead of HOST_AND_SCHEME, and ASSERTing instead of returning the clamped input if a non-local frame is encountered in WebViewImpl. We'll see how those pan out, and go from there. https://codereview.chromium.org/1804023002/diff/140001/content/browser/host_z... File content/browser/host_zoom_map_impl.cc (right): https://codereview.chromium.org/1804023002/diff/140001/content/browser/host_z... content/browser/host_zoom_map_impl.cc:287: // First, remove all entries that match the new default zoom level. On 2016/04/05 18:00:53, alexmos wrote: > Does this need to be done for scheme_host_zoom_levels_ too? (Side question: > when are those used instead of host_zoom_levels_?) In the old code-path this isn't done for scheme+host settings, so I maintained that here. In all honesty, I don't really know what the use case is for them (I don't think they're persisted in the preferences like the host-only ones). I think they may be used by printing for example. https://codereview.chromium.org/1804023002/diff/140001/content/browser/host_z... content/browser/host_zoom_map_impl.cc:306: // is different than is stored in the map. On 2016/04/05 18:00:53, alexmos wrote: > nit: s/than is stored/than the one stored/ Done. https://codereview.chromium.org/1804023002/diff/140001/content/browser/host_z... content/browser/host_zoom_map_impl.cc:319: if (GetForWebContents(web_contents) == this && uses_default_zoom) { On 2016/04/05 18:00:53, alexmos wrote: > Help me understand: why do we want to filter this by > "GetForWebContents(web_contents) == this"? Is this because HostZoomMap is > per-profile and GetAllWebContents() isn't? A comment about this would be nice. I'll add a comment. The answer is that HostZoomMaps are per-storage partition, but GetAllWebContents isn't, so we only want to change the default for WebContents that are using this storage partition. https://codereview.chromium.org/1804023002/diff/140001/content/browser/host_z... content/browser/host_zoom_map_impl.cc:323: web_contents->GetController().GetLastCommittedEntry(); On 2016/04/05 18:00:53, alexmos wrote: > It seems |entry| is already set to last committed entry above, any reason to > redo it here? Done. https://codereview.chromium.org/1804023002/diff/140001/content/browser/host_z... content/browser/host_zoom_map_impl.cc:332: zoom_level_changed_callbacks_.Notify(change); On 2016/04/05 18:00:53, alexmos wrote: > I didn't see this notification used previously for default zoom level changes. > Did I miss it, or is this a new thing you're adding? (and if so, why is it > necessary?) Also, it seems weird that change.mode is set to scheme/host whereas > this is changing default zoom. It was done previously, but via a rather roundabout route: changes in default zoom level would trigger new renderer_prefs to be sent (via a Prefs observer) to the renderer, and if the zoom level changed it would send another IPC back which would set the zoom level for the view to a value matching the default and triggering the notification. I'm not sure about the SCHEME_AND_HOST, but I'll try it with just HOST (which does make more sense). https://codereview.chromium.org/1804023002/diff/140001/content/browser/host_z... content/browser/host_zoom_map_impl.cc:532: int routing_id = web_contents->GetRenderViewHost()->GetRoutingID(); On 2016/04/05 18:00:53, alexmos wrote: > nit: render_view_id or view_routing_id might be a bit more readable. Done. Note: routing_id is in use elsewhere in the file, so at some point the var names will need to be harmonized, but that's not something we should fix in this CL. https://codereview.chromium.org/1804023002/diff/140001/content/browser/host_z... File content/browser/host_zoom_map_impl_browsertest.cc (right): https://codereview.chromium.org/1804023002/diff/140001/content/browser/host_z... content/browser/host_zoom_map_impl_browsertest.cc:62: EXPECT_TRUE(NavigateToURL(shell(), url)); On 2016/04/05 18:00:53, alexmos wrote: > It seems RunTestForURL will now redundantly navigate to this url again. Hmmm, I missed that ... I'll remove it from RunTestForURL(). https://codereview.chromium.org/1804023002/diff/140001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1804023002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:863: SendToAllFrames(new FrameMsg_SetTemporaryZoomLevel(MSG_ROUTING_NONE, level, On 2016/04/05 18:00:53, alexmos wrote: > Not being familiar with zoom, can you please explain when temporary zoom levels > are used? Just looking at the code, it seemed this is something used when the > top document is a plugin (so that plugin-specific zoom isn't saved), but for > that case I don't understand why SendToAllFrames is necessary, since plugins > shouldn't have frames. Via the extensions API, a page can have a zoom level that is independent from the HostZoomMap, but that page can be any page and thus have subframes. PluginDocs are always considered independent. I guess they are called 'temporary' as they're not persisted in the prefs; this name pre-dates my involvement with zoom work. 'IndependentZoomLevels' might be better. https://codereview.chromium.org/1804023002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:870: SendToAllFrames( On 2016/04/05 18:00:54, alexmos wrote: > I'm curious why we can't use SendPageMessage instead, which might reduce the > redundancy in page-level zoom setting on the renderer side? Or is there some > work that needs to be done on each frame (probably > LocalFrame::setPageZoomFactor), and this just makes it easy to go through all > the frames? The latter. https://codereview.chromium.org/1804023002/diff/140001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1804023002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:227: void SetTemporaryZoomLevel(double level, bool is_temporary); On 2016/04/05 18:00:54, alexmos wrote: > nit: comments explaining what the new functions are for? Done. https://codereview.chromium.org/1804023002/diff/140001/content/common/frame_m... File content/common/frame_messages.h (right): https://codereview.chromium.org/1804023002/diff/140001/content/common/frame_m... content/common/frame_messages.h:793: // Instructs the frame to set its zoom level to match the owning WebContents. On 2016/04/05 18:00:54, alexmos wrote: > The comments for these two messages are the same - might want to explain what > the difference is. Done. https://codereview.chromium.org/1804023002/diff/140001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/1804023002/diff/140001/content/renderer/rende... content/renderer/render_frame_impl.cc:1436: IPC_MESSAGE_HANDLER(FrameMsg_SetZoomLevelFromWebContents, On 2016/04/05 18:00:54, alexmos wrote: > "FromWebContents" sounded a bit weird in the name. It seems this is about > replicating page zoom level, so would something like FrameMsg_SetPageZoomLevel > be better? Sure, I'm OK with that. Done. https://codereview.chromium.org/1804023002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1804023002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3031: double WebViewImpl::setZoomLevelForFrame(LocalFrame* frame, double zoomLevel) On 2016/04/05 18:00:54, alexmos wrote: > Why is the LocalFrame version of the function needed? Why not just have one > version that takes a WebLocalFrame and convert to LocalFrame in its body? > setZoomLevel() can invoke it with mainFrameImpl() instead of > mainFrameImpl()->frame(). Because there are callers who have WebLocalFrame* (and cannot access the LocalFrame accessor in WebLocalFrameImpl, e.g. RenderFrameImpl), and there are callers who only have access to LocalFrame* (e.g. within WebViewImpl). https://codereview.chromium.org/1804023002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3052: frame->setPageZoomFactor(zoomFactor); On 2016/04/05 18:00:54, alexmos wrote: > This calls LocalFrame::setPageAndTextZoomFactors, which also updates the zoom > parameters for all local child frames: > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit.... > Isn't this redundant, since with one IPC per frame, > WebViewImpl::setZoomLevelForFrame will also be called separately for those child > frames? Yes, I suppose there is some redundancy. This could be avoided if we bail out for any local frame that is not a local root (since all frames in a page should have the same zoom), though I'm not sure how to determine that. https://codereview.chromium.org/1804023002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3058: double WebViewImpl::setZoomLevel(double zoomLevel) On 2016/04/05 18:00:54, alexmos wrote: > There seem to be some existing renderer-initiated call sites for this, such as > setCompositorDeviceScaleFactorOverride from devtools. Would those need to > separately replicate the zoom update to any other processes for the FrameTree? They might, if it has to be done for something other than the main frame. As it stands, that might still be broken for OOPIF (not sure, as all the tests seem to pass), and in the non-OOPIF case all the mainFrame cases suffice I suspect. https://codereview.chromium.org/1804023002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3065: m_zoomLevel = zoomLevel; On 2016/04/05 18:00:54, alexmos wrote: > Is it necessary to have this duplicated here in addition to what's in > setZoomLevelForFrame? I also don't see any callers caring about the return > value. m_zoomLevel is accessed elsewhere in WebViewImpl, so I'm think we have to set it here. Are you talking about removing all references to this class var and replacing them with calls to (something like) get ZoomLevelForFrame? https://codereview.chromium.org/1804023002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3069: if (mainFrame()->isWebRemoteFrame()) On 2016/04/05 18:00:54, alexmos wrote: > How much would break if we ASSERT that mainFrame() is a local frame here, rather > than leaving the escape hatch in place? I'm not sure. I'll see if it passes the bots on a dry-run, but if it doesn't, I suggest we make a separate CL out of it. My changes to WebViewImpl in this CL are meant to be as close to a pure re-factor as possible, with the intent that we can address some of these issues separately (in a hopefully much smaller CL) as needed. https://codereview.chromium.org/1804023002/diff/140001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebView.h (right): https://codereview.chromium.org/1804023002/diff/140001/third_party/WebKit/pub... third_party/WebKit/public/web/WebView.h:208: virtual double setZoomLevelForFrame(WebLocalFrame*, double zoomLevel) = 0; On 2016/04/05 18:00:54, alexmos wrote: > nit: comment Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
As a follow on to my comments above, I've been thinking about the call to frame->setPageZoomFactor(zoomFactor); and certainly I had not realized it was updating the entire local-frame sub-tree anchored from this frame. This might make it possible to (1) send page (view) level messages from WebContents (thus reducing IPCs), and (2) make essentially no changes to WebViewImpl. I need to verify that this works, but I'll look into it. Adding the ASSERT to WebViewImpl::setZoomLevel() seems to be a failure, if only from the test point of view (though there may be cases where it would happen in the browser).
On 2016/04/05 21:44:30, wjmaclean wrote: > As a follow on to my comments above, I've been thinking about the call to > > frame->setPageZoomFactor(zoomFactor); > > and certainly I had not realized it was updating the entire local-frame sub-tree > anchored from this frame. > > This might make it possible to (1) send page (view) level messages from > WebContents (thus reducing IPCs), and (2) make essentially no changes to > WebViewImpl. > > I need to verify that this works, but I'll look into it. Yes, I think it's worth checking whether this will be cleaner with page-level messages. One approach might be to modify setPageAndTextZoomFactors to work on a Frame rather than LocalFrame and walk through all descendants rather than just local ones, but only do actual work on LocalFrames. Then we can just call it once on every main frame (whether local or remote) in every process. > Adding the ASSERT to WebViewImpl::setZoomLevel() seems to be a failure, if only > from the test point of view (though there may be cases where it would happen in > the browser). Actually, looking at those failures (on PS10), they only happened on chromeos, and after checking a few random tests, they were all failing on the same codepath: #2 0x0000024da895 blink::WebViewImpl::setZoomLevel() #3 0x00000090ccd0 content::RenderViewImpl::UpdateWebViewWithDeviceScaleFactor() #4 0x00000090c9db content::RenderViewImpl::Initialize() #5 0x00000090e99e content::RenderViewImpl::Create() I'm guessing this is happening when creating swapped-out RenderViews - maybe we can just fix this path to work properly and keep the ASSERT? I'm actually curious if this is the same path that was causing https://crbug.com/596092?
https://codereview.chromium.org/1804023002/diff/140001/content/browser/host_z... File content/browser/host_zoom_map_impl.cc (right): https://codereview.chromium.org/1804023002/diff/140001/content/browser/host_z... content/browser/host_zoom_map_impl.cc:287: // First, remove all entries that match the new default zoom level. On 2016/04/05 20:21:59, wjmaclean wrote: > On 2016/04/05 18:00:53, alexmos wrote: > > Does this need to be done for scheme_host_zoom_levels_ too? (Side question: > > when are those used instead of host_zoom_levels_?) > > In the old code-path this isn't done for scheme+host settings, so I maintained > that here. In all honesty, I don't really know what the use case is for them (I > don't think they're persisted in the preferences like the host-only ones). I > think they may be used by printing for example. Where was this done in the old codepath? I saw that HostZoomMapImpl::SetZoomLevelForHost erases the levels that match default_zoom_level_, but that seems to still be present after your changes. https://codereview.chromium.org/1804023002/diff/140001/content/browser/host_z... content/browser/host_zoom_map_impl.cc:319: if (GetForWebContents(web_contents) == this && uses_default_zoom) { On 2016/04/05 20:22:00, wjmaclean wrote: > On 2016/04/05 18:00:53, alexmos wrote: > > Help me understand: why do we want to filter this by > > "GetForWebContents(web_contents) == this"? Is this because HostZoomMap is > > per-profile and GetAllWebContents() isn't? A comment about this would be > nice. > > I'll add a comment. > > The answer is that HostZoomMaps are per-storage partition, but GetAllWebContents > isn't, so we only want to change the default for WebContents that are using this > storage partition. Acknowledged. https://codereview.chromium.org/1804023002/diff/140001/content/browser/host_z... content/browser/host_zoom_map_impl.cc:332: zoom_level_changed_callbacks_.Notify(change); On 2016/04/05 20:22:00, wjmaclean wrote: > On 2016/04/05 18:00:53, alexmos wrote: > > I didn't see this notification used previously for default zoom level changes. > > > Did I miss it, or is this a new thing you're adding? (and if so, why is it > > necessary?) Also, it seems weird that change.mode is set to scheme/host > whereas > > this is changing default zoom. > > It was done previously, but via a rather roundabout route: changes in default > zoom level would trigger new renderer_prefs to be sent (via a Prefs observer) to > the renderer, and if the zoom level changed it would send another IPC back which > would set the zoom level for the view to a value matching the default and > triggering the notification. > > I'm not sure about the SCHEME_AND_HOST, but I'll try it with just HOST (which > does make more sense). Ah, I see. Was this the one that went through RenderViewHostImpl::OnDidZoomURL -> SetZoomLevelForView -> SetZoomLevelForHost? That one uses ZOOM_CHANGED_FOR_HOST, so yes, it makes sense that this now matches that. https://codereview.chromium.org/1804023002/diff/140001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1804023002/diff/140001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:863: SendToAllFrames(new FrameMsg_SetTemporaryZoomLevel(MSG_ROUTING_NONE, level, On 2016/04/05 20:22:00, wjmaclean wrote: > On 2016/04/05 18:00:53, alexmos wrote: > > Not being familiar with zoom, can you please explain when temporary zoom > levels > > are used? Just looking at the code, it seemed this is something used when the > > top document is a plugin (so that plugin-specific zoom isn't saved), but for > > that case I don't understand why SendToAllFrames is necessary, since plugins > > shouldn't have frames. > > Via the extensions API, a page can have a zoom level that is independent from > the HostZoomMap, but that page can be any page and thus have subframes. > PluginDocs are always considered independent. > > I guess they are called 'temporary' as they're not persisted in the prefs; this > name pre-dates my involvement with zoom work. 'IndependentZoomLevels' might be > better. Acknowledged. https://codereview.chromium.org/1804023002/diff/140001/content/common/frame_m... File content/common/frame_messages.h (right): https://codereview.chromium.org/1804023002/diff/140001/content/common/frame_m... content/common/frame_messages.h:793: // Instructs the frame to set its zoom level to match the owning WebContents. On 2016/04/05 20:22:00, wjmaclean wrote: > On 2016/04/05 18:00:54, alexmos wrote: > > The comments for these two messages are the same - might want to explain what > > the difference is. > > Done. They're still the same in the latest PS :) https://codereview.chromium.org/1804023002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1804023002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3065: m_zoomLevel = zoomLevel; On 2016/04/05 20:22:00, wjmaclean wrote: > On 2016/04/05 18:00:54, alexmos wrote: > > Is it necessary to have this duplicated here in addition to what's in > > setZoomLevelForFrame? I also don't see any callers caring about the return > > value. > > m_zoomLevel is accessed elsewhere in WebViewImpl, so I'm think we have to set it > here. Are you talking about removing all references to this class var and > replacing them with calls to (something like) get ZoomLevelForFrame? Sorry, I just meant that this work (clamping the zoom level to limits) also seems to be done inside setZoomLevelForFrame, and since this always calls setZoomLevelForFrame below, we end up doing it twice. I was hoping we could just delete this and rely on setZoomLevelForFrame doing it? https://codereview.chromium.org/1804023002/diff/180001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1804023002/diff/180001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:231: // Sets the zoom level for frames associated with this WebContents. nit: blank line above comment might be a bit more readable (also below) https://codereview.chromium.org/1804023002/diff/180001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:234: // matches |host| and (if non-empty) |scheme|. nit: clarify what it means to match |host| and |scheme| - e.g., "it matches" -> "the main frame's host and scheme matches" https://codereview.chromium.org/1804023002/diff/180001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebView.h (right): https://codereview.chromium.org/1804023002/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/web/WebView.h:208: // Sets the zoom level for the specified WebLocalFrame. nit: add "and its local descendants" or something like that, depending on how your page-level message exploration goes and how this ends up being used.
On 2016/04/07 00:43:28, alexmos wrote: > > messages. One approach might be to modify setPageAndTextZoomFactors to work on > a Frame rather than LocalFrame and walk through all descendants rather than just > local ones, but only do actual work on LocalFrames. Then we can just call it > once on every main frame (whether local or remote) in every process. Yes, I've done something similar already. However, the new approach has some sync issues to be sorted out before I can upload it. > I'm guessing this is happening when creating swapped-out RenderViews - maybe we > can just fix this path to work properly and keep the ASSERT? I'm actually > curious if this is the same path that was causing https://crbug.com/596092 The way I've restructured the code, the ASSERT will no longer be relevant (since one codepath will work for both local and remote frames.
I'm close to having a revised version ready (sorting out an ugly sync issue), but here are respondes to comments in the meantime. https://codereview.chromium.org/1804023002/diff/140001/content/browser/host_z... File content/browser/host_zoom_map_impl.cc (right): https://codereview.chromium.org/1804023002/diff/140001/content/browser/host_z... content/browser/host_zoom_map_impl.cc:287: // First, remove all entries that match the new default zoom level. On 2016/04/07 01:20:56, alexmos wrote: > On 2016/04/05 20:21:59, wjmaclean wrote: > > On 2016/04/05 18:00:53, alexmos wrote: > > > Does this need to be done for scheme_host_zoom_levels_ too? (Side question: > > > when are those used instead of host_zoom_levels_?) > > > > In the old code-path this isn't done for scheme+host settings, so I maintained > > that here. In all honesty, I don't really know what the use case is for them > (I > > don't think they're persisted in the preferences like the host-only ones). I > > think they may be used by printing for example. > > Where was this done in the old codepath? I saw that > HostZoomMapImpl::SetZoomLevelForHost erases the levels that match > default_zoom_level_, but that seems to still be present after your changes. As I mentioned before, the old codepath involved an arc through the renderer and back, which called SetZoomLevelForView and which subsequently called SetZoomLevelForHost. The one in SetZoomLevelForHost should remain, as it will remove entries when a page with a non-default zoom level is changed to have the default zoom level by the user. https://codereview.chromium.org/1804023002/diff/140001/content/common/frame_m... File content/common/frame_messages.h (right): https://codereview.chromium.org/1804023002/diff/140001/content/common/frame_m... content/common/frame_messages.h:793: // Instructs the frame to set its zoom level to match the owning WebContents. On 2016/04/07 01:20:56, alexmos wrote: > On 2016/04/05 20:22:00, wjmaclean wrote: > > On 2016/04/05 18:00:54, alexmos wrote: > > > The comments for these two messages are the same - might want to explain > what > > > the difference is. > > > > Done. > > They're still the same in the latest PS :) Ooops. No matter, as these messages go away in the next patch. https://codereview.chromium.org/1804023002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1804023002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3065: m_zoomLevel = zoomLevel; On 2016/04/07 01:20:56, alexmos wrote: > On 2016/04/05 20:22:00, wjmaclean wrote: > > On 2016/04/05 18:00:54, alexmos wrote: > > > Is it necessary to have this duplicated here in addition to what's in > > > setZoomLevelForFrame? I also don't see any callers caring about the return > > > value. > > > > m_zoomLevel is accessed elsewhere in WebViewImpl, so I'm think we have to set > it > > here. Are you talking about removing all references to this class var and > > replacing them with calls to (something like) get ZoomLevelForFrame? > > Sorry, I just meant that this work (clamping the zoom level to limits) also > seems to be done inside setZoomLevelForFrame, and since this always calls > setZoomLevelForFrame below, we end up doing it twice. I was hoping we could > just delete this and rely on setZoomLevelForFrame doing it? I'd be happy to do that in a follow-on if I can, but I'm really trying to avoid touching things if it's not central to the purpose of this CL. Is that ok? https://codereview.chromium.org/1804023002/diff/180001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1804023002/diff/180001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:231: // Sets the zoom level for frames associated with this WebContents. On 2016/04/07 01:20:56, alexmos wrote: > nit: blank line above comment might be a bit more readable (also below) Done. https://codereview.chromium.org/1804023002/diff/180001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:234: // matches |host| and (if non-empty) |scheme|. On 2016/04/07 01:20:56, alexmos wrote: > nit: clarify what it means to match |host| and |scheme| - e.g., "it matches" -> > "the main frame's host and scheme matches" Done. https://codereview.chromium.org/1804023002/diff/180001/third_party/WebKit/pub... File third_party/WebKit/public/web/WebView.h (right): https://codereview.chromium.org/1804023002/diff/180001/third_party/WebKit/pub... third_party/WebKit/public/web/WebView.h:208: // Sets the zoom level for the specified WebLocalFrame. On 2016/04/07 01:20:56, alexmos wrote: > nit: add "and its local descendants" or something like that, depending on how > your page-level message exploration goes and how this ends up being used. This disappears in the new patch.
Description was changed from ========== Fix page zoom to be frame-centric for out-of-process frames. This CL modifies the current methods for setting page zoom from the browser to make them work properly with out-of-process frames. HostZoomMapImpl now routes all zoom-level changes through the WebContents as the latter knows about all the frames associated with the page, regardless of which process/view they are in. This CL also modifies how default page zoom is applied, by having HostZoomMapImpl push the changes through the WebContents instead of propagating them via the RendererPrefs. This should make the code around default zoom levels easier to understand as well. It also avoids changing zoom levels on frames/views embedded in a WebContents with a custom (non-default) zoom level. BUG=528407 ========== to ========== Fix page zoom to be frame-centric for out-of-process frames. This CL modifies the current methods for setting page zoom from the browser to make them work properly with out-of-process frames. HostZoomMapImpl now routes all zoom-level changes through the WebContents as the latter knows about all the frames associated with the page, regardless of which process/view they are in. This CL also modifies how default page zoom is applied, by having HostZoomMapImpl push the changes through the WebContents instead of propagating them via the RendererPrefs. This should make the code around default zoom levels easier to understand as well. It also avoids changing zoom levels on frames/views embedded in a WebContents with a custom (non-default) zoom level. BUG=528407 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
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/1804023002/220001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804023002/220001
Description was changed from ========== Fix page zoom to be frame-centric for out-of-process frames. This CL modifies the current methods for setting page zoom from the browser to make them work properly with out-of-process frames. HostZoomMapImpl now routes all zoom-level changes through the WebContents as the latter knows about all the frames associated with the page, regardless of which process/view they are in. This CL also modifies how default page zoom is applied, by having HostZoomMapImpl push the changes through the WebContents instead of propagating them via the RendererPrefs. This should make the code around default zoom levels easier to understand as well. It also avoids changing zoom levels on frames/views embedded in a WebContents with a custom (non-default) zoom level. BUG=528407 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix page zoom to be frame-centric for out-of-process frames. This CL modifies the current methods for setting page zoom from the browser to make them work properly with out-of-process frames. HostZoomMapImpl now routes all zoom-level changes through the WebContents as the latter knows about all the frames associated with the page, regardless of which process/view they are in. This CL also modifies how default page zoom is applied, by having HostZoomMapImpl push the changes through the WebContents instead of propagating them via the RendererPrefs. This should make the code around default zoom levels easier to understand as well. It also avoids changing zoom levels on frames/views embedded in a WebContents with a custom (non-default) zoom level. BUG=528407 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
wjmaclean@chromium.org changed reviewers: + pdr@chromium.org
pdr@ - Could you please look at the Blink changes? alexmos@ - I've converted it to use PageMsg ... ptal?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
wjmaclean@chromium.org changed reviewers: - pdr@chromium.org
wjmaclean@chromium.org changed reviewers: + bokan@chromium.org
bokan@ - Would you please look at the WebViewImpl changes?
WebViewImpl lgtm! (% ASSERT->DCHECK) https://codereview.chromium.org/1804023002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1804023002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3061: ASSERT(frame->isLocalRoot()); Blink recently switched ASSERTs to DCHECKs.
Another round of comments below, most important one is the last one about a potential issue in propagateZoomToLocalFrameRoots. I like the page message approach! I'll be OOO Friday, so Nick, you can start reviewing content/ in the meantime if you'd like. https://codereview.chromium.org/1804023002/diff/140001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1804023002/diff/140001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3065: m_zoomLevel = zoomLevel; On 2016/04/07 12:55:36, wjmaclean wrote: > On 2016/04/07 01:20:56, alexmos wrote: > > On 2016/04/05 20:22:00, wjmaclean wrote: > > > On 2016/04/05 18:00:54, alexmos wrote: > > > > Is it necessary to have this duplicated here in addition to what's in > > > > setZoomLevelForFrame? I also don't see any callers caring about the > return > > > > value. > > > > > > m_zoomLevel is accessed elsewhere in WebViewImpl, so I'm think we have to > set > > it > > > here. Are you talking about removing all references to this class var and > > > replacing them with calls to (something like) get ZoomLevelForFrame? > > > > Sorry, I just meant that this work (clamping the zoom level to limits) also > > seems to be done inside setZoomLevelForFrame, and since this always calls > > setZoomLevelForFrame below, we end up doing it twice. I was hoping we could > > just delete this and rely on setZoomLevelForFrame doing it? > > I'd be happy to do that in a follow-on if I can, but I'm really trying to avoid > touching things if it's not central to the purpose of this CL. Is that ok? Yes, that's ok, though it doesn't look like this is an issue anymore in the latest PS. :) https://codereview.chromium.org/1804023002/diff/220001/content/browser/host_z... File content/browser/host_zoom_map_impl_browsertest.cc (right): https://codereview.chromium.org/1804023002/diff/220001/content/browser/host_z... content/browser/host_zoom_map_impl_browsertest.cc:79: GetZoomForView_HostAndScheme) { The existing tests don't seem to cover the OOPIF cases. Were you planning to add a new test for that? https://codereview.chromium.org/1804023002/diff/220001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1804023002/diff/220001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:341: params.preferred_subframe_zoom_level = zoom_level; It seems that some similar looking params (e.g., never_visible) are fetched from WC via the delegate_, what do you think about that approach vs. adding another param to CreateRenderView? That would avoid touching the plumbing in tests and passing 0.0 in places where it doesn't matter. https://codereview.chromium.org/1804023002/diff/220001/content/common/page_me... File content/common/page_messages.h (right): https://codereview.chromium.org/1804023002/diff/220001/content/common/page_me... content/common/page_messages.h:9: nit: why this blank line? https://codereview.chromium.org/1804023002/diff/220001/content/common/page_me... content/common/page_messages.h:20: // Create a 'type' for our enum, since IPC doesn't support enum classes (yet). Hmm, we use things like WebSandboxFlags and WebTreeScopeType in IPCs just fine, and those are enum class. Also seems like you'll need IPC_ENUM_TRAITS_MAX_VALUE for this (see examples in frame_messages.h) https://codereview.chromium.org/1804023002/diff/220001/content/common/view_me... File content/common/view_messages.h (right): https://codereview.chromium.org/1804023002/diff/220001/content/common/view_me... content/common/view_messages.h:540: // The preferred zoom level for sub-frames nit: s/sub-frames/subframes./ (just for consistency with comments elsewhere) https://codereview.chromium.org/1804023002/diff/220001/content/common/view_me... content/common/view_messages.h:540: // The preferred zoom level for sub-frames Not sure what you mean by the zoom level being "preferred"? Does that refer to the fact that subframes may not end up using it in some cases (such as for plugin documents)? The comment should probably explain this. I'm not sure "subframe" adds value either - see my other comment about this below. https://codereview.chromium.org/1804023002/diff/220001/content/renderer/rende... File content/renderer/render_view_browsertest.cc (right): https://codereview.chromium.org/1804023002/diff/220001/content/renderer/rende... content/renderer/render_view_browsertest.cc:2103: view()->webview()->setZoomLevel(zoom_level); Is this change still necessary? You've reverted SetZoomLevel to still call webview()->setZoomLevel() now. https://codereview.chromium.org/1804023002/diff/220001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/1804023002/diff/220001/content/renderer/rende... content/renderer/render_view_impl.cc:1388: IPC_MESSAGE_HANDLER(PageMsg_SetZoomLevel, OnSetZoomLevelForView) nit: might be better if the function name matched the message. https://codereview.chromium.org/1804023002/diff/220001/content/renderer/rende... content/renderer/render_view_impl.cc:2365: default: // ZOOM_USE_CURRENT_TEMPORARY_MODE why not just an explicit case for ZOOM_USE_CURRENT_TEMPORARY_MODE, and NOTREACHED() for default:? https://codereview.chromium.org/1804023002/diff/220001/content/renderer/rende... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/1804023002/diff/220001/content/renderer/rende... content/renderer/render_view_impl.h:906: // Used to indicate the zoom level of the main-frame so that subframes can nit: s/main-frame/main frame/ https://codereview.chromium.org/1804023002/diff/220001/content/renderer/rende... content/renderer/render_view_impl.h:908: double preferred_subframe_zoom_level_; I'm not sure about this name - it makes it seem like the main frame and subframe zoom levels might be different, which isn't the case. I'm also not sure "preferred" adds anything (see my earlier question). Maybe just name it page_zoom_level everywhere? Also, it might be worth mentioning more precisely how this is used, like you did below (i.e., for setting zoom on subsequent subframe loads.) https://codereview.chromium.org/1804023002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1804023002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3057: propagateZoomToLocalFrameRoots(child); Hmm, what if you have a A-B-A scenario, and you send the zoom update to A's process? It doesn't look like this will ever reach the grandchild, since the iteration in LocalFrame::setPageAndTextZoomFactors ignores remote children.
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
https://codereview.chromium.org/1804023002/diff/220001/content/browser/host_z... File content/browser/host_zoom_map_impl_browsertest.cc (right): https://codereview.chromium.org/1804023002/diff/220001/content/browser/host_z... content/browser/host_zoom_map_impl_browsertest.cc:79: GetZoomForView_HostAndScheme) { On 2016/04/07 23:48:08, alexmos (OOO until Apr 11) wrote: > The existing tests don't seem to cover the OOPIF cases. Were you planning to > add a new test for that? Yes, but I like to write tests once the rest of the CL has settled. Once I know the approach is ok, then I'll add some tests before a final LGTM. https://codereview.chromium.org/1804023002/diff/220001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1804023002/diff/220001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:341: params.preferred_subframe_zoom_level = zoom_level; On 2016/04/07 23:48:08, alexmos (OOO until Apr 11) wrote: > It seems that some similar looking params (e.g., never_visible) are fetched from > WC via the delegate_, what do you think about that approach vs. adding another > param to CreateRenderView? That would avoid touching the plumbing in tests and > passing 0.0 in places where it doesn't matter. It seemed to me that since RenderViewHostDelegate is "An interface implemented by an object interested in knowing about the state of the RenderViewHost" and since a zoom level is really a property of a WebContents, or a Page, that it didn't seem to belong there. But, I'm willing to be convinced otherwise. https://codereview.chromium.org/1804023002/diff/220001/content/common/page_me... File content/common/page_messages.h (right): https://codereview.chromium.org/1804023002/diff/220001/content/common/page_me... content/common/page_messages.h:9: On 2016/04/07 23:48:08, alexmos (OOO until Apr 11) wrote: > nit: why this blank line? Done. https://codereview.chromium.org/1804023002/diff/220001/content/common/page_me... content/common/page_messages.h:20: // Create a 'type' for our enum, since IPC doesn't support enum classes (yet). On 2016/04/07 23:48:08, alexmos (OOO until Apr 11) wrote: > Hmm, we use things like WebSandboxFlags and WebTreeScopeType in IPCs just fine, > and those are enum class. Also seems like you'll need IPC_ENUM_TRAITS_MAX_VALUE > for this (see examples in frame_messages.h) Ahh, ok. I was getting messages about the pickler not being able to generate the read/write code for these, and a colleague indicated we don't support enum classes for IPC. If we do, then that's fine too. :-) Done. https://codereview.chromium.org/1804023002/diff/220001/content/common/view_me... File content/common/view_messages.h (right): https://codereview.chromium.org/1804023002/diff/220001/content/common/view_me... content/common/view_messages.h:540: // The preferred zoom level for sub-frames On 2016/04/07 23:48:08, alexmos (OOO until Apr 11) wrote: > nit: s/sub-frames/subframes./ (just for consistency with comments elsewhere) Done. https://codereview.chromium.org/1804023002/diff/220001/content/common/view_me... content/common/view_messages.h:540: // The preferred zoom level for sub-frames On 2016/04/07 23:48:08, alexmos (OOO until Apr 11) wrote: > Not sure what you mean by the zoom level being "preferred"? Does that refer to > the fact that subframes may not end up using it in some cases (such as for > plugin documents)? The comment should probably explain this. I'm not sure > "subframe" adds value either - see my other comment about this below. You're right, we probably don't need "preferred", but subframe is useful since (at least at present), this value is only going to be used to set subframe zooms. https://codereview.chromium.org/1804023002/diff/220001/content/renderer/rende... File content/renderer/render_view_browsertest.cc (right): https://codereview.chromium.org/1804023002/diff/220001/content/renderer/rende... content/renderer/render_view_browsertest.cc:2103: view()->webview()->setZoomLevel(zoom_level); On 2016/04/07 23:48:08, alexmos (OOO until Apr 11) wrote: > Is this change still necessary? You've reverted SetZoomLevel to still call > webview()->setZoomLevel() now. Done. https://codereview.chromium.org/1804023002/diff/220001/content/renderer/rende... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/1804023002/diff/220001/content/renderer/rende... content/renderer/render_view_impl.h:906: // Used to indicate the zoom level of the main-frame so that subframes can On 2016/04/07 23:48:08, alexmos (OOO until Apr 11) wrote: > nit: s/main-frame/main frame/ Done. https://codereview.chromium.org/1804023002/diff/220001/content/renderer/rende... content/renderer/render_view_impl.h:908: double preferred_subframe_zoom_level_; On 2016/04/07 23:48:08, alexmos (OOO until Apr 11) wrote: > I'm not sure about this name - it makes it seem like the main frame and subframe > zoom levels might be different, which isn't the case. I'm also not sure > "preferred" adds anything (see my earlier question). Maybe just name it > page_zoom_level everywhere? Also, it might be worth mentioning more precisely > how this is used, like you did below (i.e., for setting zoom on subsequent > subframe loads.) Done. https://codereview.chromium.org/1804023002/diff/220001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1804023002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3057: propagateZoomToLocalFrameRoots(child); On 2016/04/07 23:48:08, alexmos (OOO until Apr 11) wrote: > Hmm, what if you have a A-B-A scenario, and you send the zoom update to A's > process? It doesn't look like this will ever reach the grandchild, since the > iteration in LocalFrame::setPageAndTextZoomFactors ignores remote children. The Zoom update will be sent to both A's and B's processes. But you're right this will miss the grandchild. Done. https://codereview.chromium.org/1804023002/diff/220001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3061: ASSERT(frame->isLocalRoot()); On 2016/04/07 22:57:12, bokan wrote: > Blink recently switched ASSERTs to DCHECKs. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1804023002/240001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804023002/240001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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/1804023002/260001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804023002/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Other than "this needs tests" I don't have any qualms with the the overall approach here (and I'm very happy to see this work with oopifs). Once alex is satisfied I'll be happy to l-g-t-m. https://codereview.chromium.org/1804023002/diff/220001/content/browser/render... File content/browser/renderer_host/render_view_host_impl.cc (right): https://codereview.chromium.org/1804023002/diff/220001/content/browser/render... content/browser/renderer_host/render_view_host_impl.cc:341: params.preferred_subframe_zoom_level = zoom_level; On 2016/04/08 20:13:29, wjmaclean wrote: > On 2016/04/07 23:48:08, alexmos (OOO until Apr 11) wrote: > > It seems that some similar looking params (e.g., never_visible) are fetched > from > > WC via the delegate_, what do you think about that approach vs. adding another > > param to CreateRenderView? That would avoid touching the plumbing in tests > and > > passing 0.0 in places where it doesn't matter. > > It seemed to me that since RenderViewHostDelegate is "An interface implemented > by an object interested in knowing about the state of the RenderViewHost" and > since a zoom level is really a property of a WebContents, or a Page, that it > didn't seem to belong there. > > But, I'm willing to be convinced otherwise. I'm okay with either approach. Practically, if you do it via the delegate it's harder to get things to work in intersitials. But it looks like zoom doesn't work properly today in interstitials anyway. https://codereview.chromium.org/1804023002/diff/220001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1804023002/diff/220001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:863: void WebContentsImpl::SetTemporaryZoomLevel(double level, bool is_temporary) { is_temporary -> temporary_zoom_enabled https://codereview.chromium.org/1804023002/diff/220001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/1804023002/diff/220001/content/renderer/rende... content/renderer/render_view_impl.cc:1388: IPC_MESSAGE_HANDLER(PageMsg_SetZoomLevel, OnSetZoomLevelForView) On 2016/04/07 23:48:08, alexmos wrote: > nit: might be better if the function name matched the message. Agree with this. https://codereview.chromium.org/1804023002/diff/260001/content/browser/host_z... File content/browser/host_zoom_map_impl.cc (right): https://codereview.chromium.org/1804023002/diff/260001/content/browser/host_z... content/browser/host_zoom_map_impl.cc:316: !HasZoomLevel(url.scheme(), net::GetHostOrSpecFromURL(url)) && |url| may be empty here per the above comment. In that case GetHostOrSpecFromURL would DCHECK before returning an empty string. How hard would it be to write a test to exercise this case? https://codereview.chromium.org/1804023002/diff/260001/content/browser/host_z... content/browser/host_zoom_map_impl.cc:321: if (GetForWebContents(web_contents) == this && uses_default_zoom) { optional suggestion: consider moving the "GetForWebContents(web_contents) == this" filtering up to the top of the loop: if (GetForWebContents(web_contents) != this) continue; https://codereview.chromium.org/1804023002/diff/260001/content/browser/host_z... content/browser/host_zoom_map_impl.cc:327: entry ? net::GetHostOrSpecFromURL(HostZoomMap::GetURLFromEntry(entry)) Seems like this is already computed, as |url|. https://codereview.chromium.org/1804023002/diff/260001/content/browser/host_z... content/browser/host_zoom_map_impl.cc:533: if (GetForWebContents(web_contents) == this && If you update the other loop to use a 'continue:', update this too. https://codereview.chromium.org/1804023002/diff/260001/content/browser/render... File content/browser/renderer_host/render_view_host_delegate.h (right): https://codereview.chromium.org/1804023002/diff/260001/content/browser/render... content/browser/renderer_host/render_view_host_delegate.h:224: virtual double PageZoomLevel(); Rename to GetPageZoomLevel https://codereview.chromium.org/1804023002/diff/260001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1804023002/diff/260001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:890: (!scheme.empty() && scheme != url.scheme())) { (!scheme.empty() && !url.SchemeIs(scheme))
A couple more nits, but overall this is looking good. Thanks for all the work on this! I'm mostly just waiting for tests now. Also, it seems that WebViewTests_WebViewTest.Shim_TestZoomAPI_1 is failing on the asan bot? https://codereview.chromium.org/1804023002/diff/220001/content/common/view_me... File content/common/view_messages.h (right): https://codereview.chromium.org/1804023002/diff/220001/content/common/view_me... content/common/view_messages.h:540: // The preferred zoom level for sub-frames On 2016/04/08 20:13:29, wjmaclean wrote: > On 2016/04/07 23:48:08, alexmos (OOO until Apr 11) wrote: > > Not sure what you mean by the zoom level being "preferred"? Does that refer > to > > the fact that subframes may not end up using it in some cases (such as for > > plugin documents)? The comment should probably explain this. I'm not sure > > "subframe" adds value either - see my other comment about this below. > > You're right, we probably don't need "preferred", but subframe is useful since > (at least at present), this value is only going to be used to set subframe > zooms. OK. The comment still has "preferred" in it, so might want to update that to say it's "page zoom level shared by all frames on this page." https://codereview.chromium.org/1804023002/diff/220001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/1804023002/diff/220001/content/renderer/rende... content/renderer/render_view_impl.cc:2365: default: // ZOOM_USE_CURRENT_TEMPORARY_MODE On 2016/04/07 23:48:08, alexmos wrote: > why not just an explicit case for ZOOM_USE_CURRENT_TEMPORARY_MODE, and > NOTREACHED() for default:? Still curious about this. :) https://codereview.chromium.org/1804023002/diff/240001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1804023002/diff/240001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:444: double PageZoomLevel() override; nit: put in the RenderViewHostDelegate section. Currently it's under RenderFrameHostDelegate. https://codereview.chromium.org/1804023002/diff/240001/content/renderer/rende... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/1804023002/diff/240001/content/renderer/rende... content/renderer/render_view_impl.h:906: // Used to indicate the zoom level to be used during subframes loads, since nit: s/subframes/subframe/ https://codereview.chromium.org/1804023002/diff/260001/content/common/page_me... File content/common/page_message_enums.h (right): https://codereview.chromium.org/1804023002/diff/260001/content/common/page_me... content/common/page_message_enums.h:9: ZOOM_SET_TEMPORARY, nit: ZOOM_ prefix seems redundant now that this is an enum class. https://codereview.chromium.org/1804023002/diff/260001/content/common/page_me... File content/common/page_messages.h (right): https://codereview.chromium.org/1804023002/diff/260001/content/common/page_me... content/common/page_messages.h:19: PageMsg_SetZoomLevel_Command::ZOOM_USE_CURRENT_TEMPORARY_MODE); It might be a good idea to define a PageMsg_SetZoomLevel_Command::LAST (= USE_CURRENT_TEMPORARY_MODE in enum class) and use it here. That way this won't get out of date if a new command is added to the enum. Many, though not all, frame and view messages seem to use this pattern, so I'm guessing this is not required but nice to have.
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/1804023002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804023002/280001
alexmos@ - Comments addressed, but no need to review this patch (it's primarily to run on the bots, and I still have to add testing). https://codereview.chromium.org/1804023002/diff/220001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1804023002/diff/220001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:863: void WebContentsImpl::SetTemporaryZoomLevel(double level, bool is_temporary) { On 2016/04/11 22:17:03, ncarter wrote: > is_temporary -> temporary_zoom_enabled Done. https://codereview.chromium.org/1804023002/diff/220001/content/common/view_me... File content/common/view_messages.h (right): https://codereview.chromium.org/1804023002/diff/220001/content/common/view_me... content/common/view_messages.h:540: // The preferred zoom level for sub-frames On 2016/04/11 23:19:10, alexmos wrote: > On 2016/04/08 20:13:29, wjmaclean wrote: > > On 2016/04/07 23:48:08, alexmos (OOO until Apr 11) wrote: > > > Not sure what you mean by the zoom level being "preferred"? Does that refer > > to > > > the fact that subframes may not end up using it in some cases (such as for > > > plugin documents)? The comment should probably explain this. I'm not sure > > > "subframe" adds value either - see my other comment about this below. > > > > You're right, we probably don't need "preferred", but subframe is useful since > > (at least at present), this value is only going to be used to set subframe > > zooms. > > OK. The comment still has "preferred" in it, so might want to update that to > say it's "page zoom level shared by all frames on this page." Done. https://codereview.chromium.org/1804023002/diff/220001/content/renderer/rende... File content/renderer/render_view_impl.cc (right): https://codereview.chromium.org/1804023002/diff/220001/content/renderer/rende... content/renderer/render_view_impl.cc:1388: IPC_MESSAGE_HANDLER(PageMsg_SetZoomLevel, OnSetZoomLevelForView) On 2016/04/07 23:48:08, alexmos wrote: > nit: might be better if the function name matched the message. Done. https://codereview.chromium.org/1804023002/diff/220001/content/renderer/rende... content/renderer/render_view_impl.cc:2365: default: // ZOOM_USE_CURRENT_TEMPORARY_MODE On 2016/04/07 23:48:08, alexmos wrote: > why not just an explicit case for ZOOM_USE_CURRENT_TEMPORARY_MODE, and > NOTREACHED() for default:? I guess I just figured less code generated, but no worries. Done. https://codereview.chromium.org/1804023002/diff/220001/content/renderer/rende... content/renderer/render_view_impl.cc:2365: default: // ZOOM_USE_CURRENT_TEMPORARY_MODE On 2016/04/11 23:19:10, alexmos wrote: > On 2016/04/07 23:48:08, alexmos wrote: > > why not just an explicit case for ZOOM_USE_CURRENT_TEMPORARY_MODE, and > > NOTREACHED() for default:? > > Still curious about this. :) I didn't see this comment the first time. https://codereview.chromium.org/1804023002/diff/240001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/1804023002/diff/240001/content/browser/web_co... content/browser/web_contents/web_contents_impl.h:444: double PageZoomLevel() override; On 2016/04/11 23:19:10, alexmos wrote: > nit: put in the RenderViewHostDelegate section. Currently it's under > RenderFrameHostDelegate. Done. https://codereview.chromium.org/1804023002/diff/240001/content/renderer/rende... File content/renderer/render_view_impl.h (right): https://codereview.chromium.org/1804023002/diff/240001/content/renderer/rende... content/renderer/render_view_impl.h:906: // Used to indicate the zoom level to be used during subframes loads, since On 2016/04/11 23:19:10, alexmos wrote: > nit: s/subframes/subframe/ Done. https://codereview.chromium.org/1804023002/diff/260001/content/browser/host_z... File content/browser/host_zoom_map_impl.cc (right): https://codereview.chromium.org/1804023002/diff/260001/content/browser/host_z... content/browser/host_zoom_map_impl.cc:316: !HasZoomLevel(url.scheme(), net::GetHostOrSpecFromURL(url)) && On 2016/04/11 22:17:03, ncarter wrote: > |url| may be empty here per the above comment. In that case GetHostOrSpecFromURL > would DCHECK before returning an empty string. > > How hard would it be to write a test to exercise this case? Perhaps just easier to code it so that doesn't matter? https://codereview.chromium.org/1804023002/diff/260001/content/browser/host_z... content/browser/host_zoom_map_impl.cc:321: if (GetForWebContents(web_contents) == this && uses_default_zoom) { On 2016/04/11 22:17:03, ncarter wrote: > optional suggestion: consider moving the "GetForWebContents(web_contents) == > this" filtering up to the top of the loop: > > if (GetForWebContents(web_contents) != this) > continue; Done. https://codereview.chromium.org/1804023002/diff/260001/content/browser/host_z... content/browser/host_zoom_map_impl.cc:327: entry ? net::GetHostOrSpecFromURL(HostZoomMap::GetURLFromEntry(entry)) On 2016/04/11 22:17:03, ncarter wrote: > Seems like this is already computed, as |url|. Done. https://codereview.chromium.org/1804023002/diff/260001/content/browser/host_z... content/browser/host_zoom_map_impl.cc:533: if (GetForWebContents(web_contents) == this && On 2016/04/11 22:17:03, ncarter wrote: > If you update the other loop to use a 'continue:', update this too. Done. https://codereview.chromium.org/1804023002/diff/260001/content/browser/render... File content/browser/renderer_host/render_view_host_delegate.h (right): https://codereview.chromium.org/1804023002/diff/260001/content/browser/render... content/browser/renderer_host/render_view_host_delegate.h:224: virtual double PageZoomLevel(); On 2016/04/11 22:17:03, ncarter wrote: > Rename to GetPageZoomLevel Done. https://codereview.chromium.org/1804023002/diff/260001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1804023002/diff/260001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:890: (!scheme.empty() && scheme != url.scheme())) { On 2016/04/11 22:17:03, ncarter wrote: > (!scheme.empty() && !url.SchemeIs(scheme)) Done. https://codereview.chromium.org/1804023002/diff/260001/content/common/page_me... File content/common/page_message_enums.h (right): https://codereview.chromium.org/1804023002/diff/260001/content/common/page_me... content/common/page_message_enums.h:9: ZOOM_SET_TEMPORARY, On 2016/04/11 23:19:10, alexmos wrote: > nit: ZOOM_ prefix seems redundant now that this is an enum class. Done. https://codereview.chromium.org/1804023002/diff/260001/content/common/page_me... File content/common/page_messages.h (right): https://codereview.chromium.org/1804023002/diff/260001/content/common/page_me... content/common/page_messages.h:19: PageMsg_SetZoomLevel_Command::ZOOM_USE_CURRENT_TEMPORARY_MODE); On 2016/04/11 23:19:11, alexmos wrote: > It might be a good idea to define a PageMsg_SetZoomLevel_Command::LAST (= > USE_CURRENT_TEMPORARY_MODE in enum class) and use it here. That way this won't > get out of date if a new command is added to the enum. Many, though not all, > frame and view messages seem to use this pattern, so I'm guessing this is not > required but nice to have. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
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/1804023002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804023002/300001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
No issues with the ipc messages, but I have a comment on the Blink changes. https://codereview.chromium.org/1804023002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1804023002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3055: void WebViewImpl::propagateZoomToLocalFrameRoots(Frame* frame) We've tried to avoid doing this kind of frame walk in the past. Instead, can we add a listener list of local frame roots? There might be another cases where we need to update frame roots based on received page messages.
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/1804023002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804023002/320001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
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/1804023002/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804023002/340001
Patchset #17 (id:320001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_linux on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
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/1804023002/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804023002/360001
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/1804023002/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804023002/380001
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...)
alexmos@ - could you please take a look at the current state of the new tests? I still have to see why the top-level frame zoom in SubframesDoNotZoomIndependently test is failing on Android, but I'd like some validation of the approach before I spend anymore time on this.
On 2016/04/20 at 18:51:10, wjmaclean wrote: > alexmos@ - could you please take a look at the current state of the new tests? > > I still have to see why the top-level frame zoom in SubframesDoNotZoomIndependently test is failing on Android, but I'd like some validation of the approach before I spend anymore time on this. Btw, I'm not sure whether or not the mac 10.10 test failures are related, trying to figure that out.
Thanks! A couple of questions on the tests below and a few nits. https://codereview.chromium.org/1804023002/diff/380001/content/browser/iframe... File content/browser/iframe_zoom_browsertest.cc (right): https://codereview.chromium.org/1804023002/diff/380001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:83: auto subframe1 = root->child_at(0)->current_frame_host(); \ I'd spell out the RenderFrameHost* type for better readability. Otherwise someone might think it's a FrameTreeNode* later on and misuse it after a navigation. https://codereview.chromium.org/1804023002/diff/380001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:83: auto subframe1 = root->child_at(0)->current_frame_host(); \ "child" and "grandchild" might be better names, since subframe1 and subframe2 seem like they are siblings (this is the convention we've loosely followed in other tests). https://codereview.chromium.org/1804023002/diff/380001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:84: auto subframe2 = root->child_at(0)->child_at(0)->current_frame_host(); \ nit: subframe1->child_at(0)->current_frame_host() https://codereview.chromium.org/1804023002/diff/380001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:99: FrameResizeObserver(RenderFrameHost* adapter, nit: rename adapter to host or frame or similar. adapter doesn't really make sense here, as this always just takes a RFH* (as opposed to ToRenderFrameHost). https://codereview.chromium.org/1804023002/diff/380001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:102: double tol) nit: s/tol/tolerance/ https://codereview.chromium.org/1804023002/diff/380001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:127: resized = std::abs(expected_inner_width - inner_width) < tolerance; So this won't check whether the subframes zoomed to the correct level, just whether they did or did not zoom? Did the idea with assigning percentage zoom width to subframes and then measuring the window.innerWidth change not work out? That would allow verifying actual zoom level as well. Perhaps we could try that for just A-B if A-B-A is too complicated. Also, this seems backwards to me: if the width change was below tolerance, resized is set to true? (I'd expect it to be false) I'm a bit confused about how this would work. It seems the inner widths for subframes won't change regardless of whether or not they are zoomed (unless you also did the percentage widths, and then the resized value would be flipped). Is this only about receiving the onresize event? https://codereview.chromium.org/1804023002/diff/380001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:236: } So far, these are are about A-B-A without subframe navigations, but I think we should test at least a couple of other scenarios: - A-embed-B, zoom the page, and navigate subframe B cross-site to C. Make sure C still has correct zoom. - A embeds two children (B, B). Make sure both subframes have correct zoom.
alexmos@ - please see my replies to your major comments below, and let me know what you think; we need to be on the same page before I proceed. Any of the minor comments (the ones I haven't explicitly replied to here) I will address in the next patch-set. I'm OOO next week, so this CL looks like it won't be landable until the week after. https://codereview.chromium.org/1804023002/diff/380001/content/browser/iframe... File content/browser/iframe_zoom_browsertest.cc (right): https://codereview.chromium.org/1804023002/diff/380001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:127: resized = std::abs(expected_inner_width - inner_width) < tolerance; On 2016/04/21 at 17:35:35, alexmos wrote: > So this won't check whether the subframes zoomed to the correct level, just whether they did or did not zoom? Did the idea with assigning percentage zoom width to subframes and then measuring the window.innerWidth change not work out? That would allow verifying actual zoom level as well. Perhaps we could try that for just A-B if A-B-A is too complicated. I think you've got it backwards: this method allows for a *precise* check that the correct zoom level was achieved, since the innerWidth has to match exactly (the scale change within the frame cancels out the size change of the frameRect, leading to constant innerWidth). The other method, with percentage-width, kept leading to slight changes in innerWidth that were too hard to reason about, which is why I abandoned it. Even for a single sub-frame it was annoying hard to reason about. > Also, this seems backwards to me: if the width change was below tolerance, resized is set to true? (I'd expect it to be false) I'm a bit confused about how this would work. It seems the inner widths for subframes won't change regardless of whether or not they are zoomed (unless you also did the percentage widths, and then the resized value would be flipped). Is this only about receiving the onresize event? We expect the width change to be zero, give-or-take any floating point error. Hence change below tolerance is accepted. Here I've specified .1 pixels, which should be fine. https://codereview.chromium.org/1804023002/diff/380001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:236: } On 2016/04/21 at 17:35:35, alexmos wrote: > So far, these are are about A-B-A without subframe navigations, but I think we should test at least a couple of other scenarios: > - A-embed-B, zoom the page, and navigate subframe B cross-site to C. Make sure C still has correct zoom. > - A embeds two children (B, B). Make sure both subframes have correct zoom. I'm ok with adding some tests to test subframe navigation, and the two-child case ... I'll add these.
https://codereview.chromium.org/1804023002/diff/380001/content/browser/iframe... File content/browser/iframe_zoom_browsertest.cc (right): https://codereview.chromium.org/1804023002/diff/380001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:127: resized = std::abs(expected_inner_width - inner_width) < tolerance; On 2016/04/21 17:59:35, wjmaclean wrote: > On 2016/04/21 at 17:35:35, alexmos wrote: > > So this won't check whether the subframes zoomed to the correct level, just > whether they did or did not zoom? Did the idea with assigning percentage zoom > width to subframes and then measuring the window.innerWidth change not work out? > That would allow verifying actual zoom level as well. Perhaps we could try > that for just A-B if A-B-A is too complicated. > > I think you've got it backwards: this method allows for a *precise* check that > the correct zoom level was achieved, since the innerWidth has to match exactly > (the scale change within the frame cancels out the size change of the frameRect, > leading to constant innerWidth). The other method, with percentage-width, kept > leading to slight changes in innerWidth that were too hard to reason about, > which is why I abandoned it. Even for a single sub-frame it was annoying hard to > reason about. Ah, I see. Thanks for clarifying! (Let's explain this strategy in test comments.) I assume before any changes in your CL, if you zoom a page with an OOPIF, the OOPIF's window.innerWidth would keep increasing, since the frameRect size change is propagated but the scale isn't. I like this. Is it guaranteed that the onresize event will always fire for zoom? Also, there's a danger that a spurious onresize event (unrelated to zoom) might cause the test to still pass if the width didn't change at all, but I suppose that shouldn't be possible with the test setup? > > > Also, this seems backwards to me: if the width change was below tolerance, > resized is set to true? (I'd expect it to be false) I'm a bit confused about > how this would work. It seems the inner widths for subframes won't change > regardless of whether or not they are zoomed (unless you also did the percentage > widths, and then the resized value would be flipped). Is this only about > receiving the onresize event? > > We expect the width change to be zero, give-or-take any floating point error. > Hence change below tolerance is accepted. Here I've specified .1 pixels, which > should be fine. > Acknowledged.
https://codereview.chromium.org/1804023002/diff/380001/content/browser/iframe... File content/browser/iframe_zoom_browsertest.cc (right): https://codereview.chromium.org/1804023002/diff/380001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:127: resized = std::abs(expected_inner_width - inner_width) < tolerance; Also, maybe rename |resized| to |zoomed_correctly| or something like that, since this was what caused my confusion initially (this is about whether or not the frame maintained its old innerWidth after zoom, not about whether or not it resized).
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/1804023002/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804023002/400001
https://codereview.chromium.org/1804023002/diff/380001/content/browser/iframe... File content/browser/iframe_zoom_browsertest.cc (right): https://codereview.chromium.org/1804023002/diff/380001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:83: auto subframe1 = root->child_at(0)->current_frame_host(); \ On 2016/04/21 at 17:35:35, alexmos wrote: > I'd spell out the RenderFrameHost* type for better readability. Otherwise someone might think it's a FrameTreeNode* later on and misuse it after a navigation. Done & Done. https://codereview.chromium.org/1804023002/diff/380001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:84: auto subframe2 = root->child_at(0)->child_at(0)->current_frame_host(); \ On 2016/04/21 at 17:35:35, alexmos wrote: > nit: subframe1->child_at(0)->current_frame_host() subframe1 is a RenderFrameHostImpl*, so this doesn't work :-) https://codereview.chromium.org/1804023002/diff/380001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:99: FrameResizeObserver(RenderFrameHost* adapter, On 2016/04/21 at 17:35:35, alexmos wrote: > nit: rename adapter to host or frame or similar. adapter doesn't really make sense here, as this always just takes a RFH* (as opposed to ToRenderFrameHost). Done. https://codereview.chromium.org/1804023002/diff/380001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:102: double tol) On 2016/04/21 at 17:35:35, alexmos wrote: > nit: s/tol/tolerance/ Done. https://codereview.chromium.org/1804023002/diff/380001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:127: resized = std::abs(expected_inner_width - inner_width) < tolerance; On 2016/04/21 at 19:02:40, alexmos wrote: > On 2016/04/21 17:59:35, wjmaclean wrote: > > On 2016/04/21 at 17:35:35, alexmos wrote: > > > So this won't check whether the subframes zoomed to the correct level, just > > whether they did or did not zoom? Did the idea with assigning percentage zoom > > width to subframes and then measuring the window.innerWidth change not work out? > > That would allow verifying actual zoom level as well. Perhaps we could try > > that for just A-B if A-B-A is too complicated. > > > > I think you've got it backwards: this method allows for a *precise* check that > > the correct zoom level was achieved, since the innerWidth has to match exactly > > (the scale change within the frame cancels out the size change of the frameRect, > > leading to constant innerWidth). The other method, with percentage-width, kept > > leading to slight changes in innerWidth that were too hard to reason about, > > which is why I abandoned it. Even for a single sub-frame it was annoying hard to > > reason about. > > Ah, I see. Thanks for clarifying! (Let's explain this strategy in test comments.) I assume before any changes in your CL, if you zoom a page with an OOPIF, the OOPIF's window.innerWidth would keep increasing, since the frameRect size change is propagated but the scale isn't. > > I like this. Is it guaranteed that the onresize event will always fire for zoom? Also, there's a danger that a spurious onresize event (unrelated to zoom) might cause the test to still pass if the width didn't change at all, but I suppose that shouldn't be possible with the test setup? It *seems* like it always fires ... but I can't point to a codepath that provides a hard guarantee. But I suspect any call to setFrameRect() will force an onresize event. A spurious onresize would only cause us grief if it happened to cause the innerWidth to be our expected value, which I don't think can happen spuriously. Depending on timing, we may get one or two onresize events, but it's only the last one that has the correct size we are looking for.
Patchset #22 (id:440001) has been deleted
Patchset #22 (id:460001) has been deleted
Thanks for the new tests! A few more minor comments, but overall I think this is close. Keep us posted about what you find about Android. https://codereview.chromium.org/1804023002/diff/380001/content/browser/iframe... File content/browser/iframe_zoom_browsertest.cc (right): https://codereview.chromium.org/1804023002/diff/380001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:84: auto subframe2 = root->child_at(0)->child_at(0)->current_frame_host(); \ On 2016/04/22 19:57:01, wjmaclean wrote: > On 2016/04/21 at 17:35:35, alexmos wrote: > > nit: subframe1->child_at(0)->current_frame_host() > > subframe1 is a RenderFrameHostImpl*, so this doesn't work :-) Acknowledged. I'm too used to these kinds of things being FrameTreeNodes in our other tests. :) https://codereview.chromium.org/1804023002/diff/380001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:127: resized = std::abs(expected_inner_width - inner_width) < tolerance; On 2016/04/22 19:57:01, wjmaclean wrote: > On 2016/04/21 at 19:02:40, alexmos wrote: > > On 2016/04/21 17:59:35, wjmaclean wrote: > > > On 2016/04/21 at 17:35:35, alexmos wrote: > > > > So this won't check whether the subframes zoomed to the correct level, > just > > > whether they did or did not zoom? Did the idea with assigning percentage > zoom > > > width to subframes and then measuring the window.innerWidth change not work > out? > > > That would allow verifying actual zoom level as well. Perhaps we could try > > > that for just A-B if A-B-A is too complicated. > > > > > > I think you've got it backwards: this method allows for a *precise* check > that > > > the correct zoom level was achieved, since the innerWidth has to match > exactly > > > (the scale change within the frame cancels out the size change of the > frameRect, > > > leading to constant innerWidth). The other method, with percentage-width, > kept > > > leading to slight changes in innerWidth that were too hard to reason about, > > > which is why I abandoned it. Even for a single sub-frame it was annoying > hard to > > > reason about. > > > > Ah, I see. Thanks for clarifying! (Let's explain this strategy in test > comments.) I assume before any changes in your CL, if you zoom a page with an > OOPIF, the OOPIF's window.innerWidth would keep increasing, since the frameRect > size change is propagated but the scale isn't. > > > > I like this. Is it guaranteed that the onresize event will always fire for > zoom? Also, there's a danger that a spurious onresize event (unrelated to zoom) > might cause the test to still pass if the width didn't change at all, but I > suppose that shouldn't be possible with the test setup? > > It *seems* like it always fires ... but I can't point to a codepath that > provides a hard guarantee. But I suspect any call to setFrameRect() will force > an onresize event. A spurious onresize would only cause us grief if it happened > to cause the innerWidth to be our expected value, which I don't think can happen > spuriously. Depending on timing, we may get one or two onresize events, but it's > only the last one that has the correct size we are looking for. Acknowledged. It might be good to document the fact that we could get one or two onresize events and that we need to wait for the last one on top of the WaitForMessage loop. https://codereview.chromium.org/1804023002/diff/400001/content/browser/iframe... File content/browser/iframe_zoom_browsertest.cc (right): https://codereview.chromium.org/1804023002/diff/400001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:25: class IFrameZoomBrowserTest : public ContentBrowserTest { A comment with a high-level overview of how these tests work would be great to have here. This can just contain what you wrote in the comments (i.e., that scale changes within a frame cancel out the size change of the frameRect, leading to constant innerWidth). https://codereview.chromium.org/1804023002/diff/400001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:51: double GetMainframeZoomFactor(const ToRenderFrameHost& adapter, double border) { nit: s/GetMainframeZoomFactor/GetMainFrameZoomFactor/ for naming consistency in other places. Same below for kGetMainFrameZoomLevel. https://codereview.chromium.org/1804023002/diff/400001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:71: #define SETUP_A_B_A_NESTED_FRAMES() \ I'm not sure how I feel about this macro. :) I'm concerned it makes the tests harder to follow/modify and will screw up the reported line numbers if one of the assertions fails. Could we just repeat the setup in three tests? You're already doing it in the last two, plus see my comment about not needing the |entry| verification everywhere. Also, not sure it's worth it, but we could extract out the bits common to all five tests (main_frame_window_border and default_zoom_level verification) into a class-level helper/fields on IFrameZoomBrowserTest to reduce some of the repetition. Actually, the other global helpers might make more sense as class helpers too -- or you might want to move them to an anonymous namespace, which seems to be the typical pattern in other test files. https://codereview.chromium.org/1804023002/diff/400001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:256: EXPECT_EQ(top_level_host, loaded_url.host()); Maybe it's enough to have coverage for lines 252-256 in one of the tests? It might be a bit redundant in all tests. https://codereview.chromium.org/1804023002/diff/400001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:263: /* The following calls must be made when the page's scale factor = 1.0.*/ Any reason not to use // instead of /* */ here for consistency? Same below. https://codereview.chromium.org/1804023002/diff/400001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:279: const double kTolerance = 0.1; How about moving this to be declared only once somewhere on top? And also adding a comment that this is in pixels. https://codereview.chromium.org/1804023002/diff/400001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:324: const double new_zoom_factor = 2.5; It probably doesn't make any difference, but maybe it's worth covering the zoom factor being less than 1.0 in one of the tests? https://codereview.chromium.org/1804023002/diff/400001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:344: // Navigate child frame, and make sure zoom is the same. nit: "Navigate the child frame cross-site" https://codereview.chromium.org/1804023002/diff/400001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:352: // even if its domain has a different value specified. nit: "Check that the child frame maintained the same scale after navigating cross-site."
Patchset #22 (id:480001) has been deleted
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/1804023002/500001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804023002/500001
Comments addressed. Also, I've excluded the top-level navigation for one of the tests on Android, since Android doesn't set top-level page zoom for loading pages. PTAL https://codereview.chromium.org/1804023002/diff/400001/content/browser/iframe... File content/browser/iframe_zoom_browsertest.cc (right): https://codereview.chromium.org/1804023002/diff/400001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:25: class IFrameZoomBrowserTest : public ContentBrowserTest { On 2016/04/25 at 22:43:27, alexmos wrote: > A comment with a high-level overview of how these tests work would be great to have here. This can just contain what you wrote in the comments (i.e., that scale changes within a frame cancel out the size change of the frameRect, leading to constant innerWidth). I've tried to improve this ... let me know what you think. https://codereview.chromium.org/1804023002/diff/400001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:51: double GetMainframeZoomFactor(const ToRenderFrameHost& adapter, double border) { On 2016/04/25 at 22:43:27, alexmos wrote: > nit: s/GetMainframeZoomFactor/GetMainFrameZoomFactor/ for naming consistency in other places. Same below for kGetMainFrameZoomLevel. Done. I had used 'Mainframe' to match 'Subframe', but whatever. https://codereview.chromium.org/1804023002/diff/400001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:71: #define SETUP_A_B_A_NESTED_FRAMES() \ On 2016/04/25 at 22:43:27, alexmos wrote: > I'm not sure how I feel about this macro. :) I'm concerned it makes the tests harder to follow/modify and will screw up the reported line numbers if one of the assertions fails. Could we just repeat the setup in three tests? You're already doing it in the last two, plus see my comment about not needing the |entry| verification everywhere. Also, not sure it's worth it, but we could extract out the bits common to all five tests (main_frame_window_border and default_zoom_level verification) into a class-level helper/fields on IFrameZoomBrowserTest to reduce some of the repetition. > > Actually, the other global helpers might make more sense as class helpers too -- or you might want to move them to an anonymous namespace, which seems to be the typical pattern in other test files. I've replicated the macro, and moved the helpers into an anonymous namespace. I didn't add anything to the main class, as I don't think it's right to load the class down with stuff that's specific to certain tests. https://codereview.chromium.org/1804023002/diff/400001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:263: /* The following calls must be made when the page's scale factor = 1.0.*/ On 2016/04/25 at 22:43:27, alexmos wrote: > Any reason not to use // instead of /* */ here for consistency? Same below. This migrated from a macro, hence the old-style comment. Done. https://codereview.chromium.org/1804023002/diff/400001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:279: const double kTolerance = 0.1; On 2016/04/25 at 22:43:27, alexmos wrote: > How about moving this to be declared only once somewhere on top? And also adding a comment that this is in pixels. Done. https://codereview.chromium.org/1804023002/diff/400001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:324: const double new_zoom_factor = 2.5; On 2016/04/25 at 22:43:27, alexmos wrote: > It probably doesn't make any difference, but maybe it's worth covering the zoom factor being less than 1.0 in one of the tests? Actually, I don't think it makes any difference, as the zoom system never really looks at this value, other than to test when it equals the default zoom level. But I'll change one test anyways. https://codereview.chromium.org/1804023002/diff/400001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:344: // Navigate child frame, and make sure zoom is the same. On 2016/04/25 at 22:43:27, alexmos wrote: > nit: "Navigate the child frame cross-site" Done. https://codereview.chromium.org/1804023002/diff/400001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:352: // even if its domain has a different value specified. On 2016/04/25 at 22:43:27, alexmos wrote: > nit: "Check that the child frame maintained the same scale after navigating cross-site." Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Great! A couple of last comments and then it should be good to go from my end. https://codereview.chromium.org/1804023002/diff/300001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1804023002/diff/300001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3055: void WebViewImpl::propagateZoomToLocalFrameRoots(Frame* frame) On 2016/04/14 18:19:52, kenrb wrote: > We've tried to avoid doing this kind of frame walk in the past. Instead, can we > add a listener list of local frame roots? There might be another cases where we > need to update frame roots based on received page messages. Pinging this so that we don't forget to discuss it. :) I'll just add that this isn't really introducing a new frame walk. Previously, LocalFrame::setPageAndTextZoomFactors would walk through the entire frame tree, and this essentially partitions that walk into two parts (walk through local roots, walk through all local frames in each). But having a cleaner way to do work on all local roots would be good indeed (I'm ok if this happens in a follow-up, but I'll defer to Ken). https://codereview.chromium.org/1804023002/diff/400001/content/browser/iframe... File content/browser/iframe_zoom_browsertest.cc (right): https://codereview.chromium.org/1804023002/diff/400001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:25: class IFrameZoomBrowserTest : public ContentBrowserTest { On 2016/04/27 13:43:57, wjmaclean wrote: > On 2016/04/25 at 22:43:27, alexmos wrote: > > A comment with a high-level overview of how these tests work would be great to > have here. This can just contain what you wrote in the comments (i.e., that > scale changes within a frame cancel out the size change of the frameRect, > leading to constant innerWidth). > > I've tried to improve this ... let me know what you think. This is great - thanks for adding this! https://codereview.chromium.org/1804023002/diff/400001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:256: EXPECT_EQ(top_level_host, loaded_url.host()); On 2016/04/25 22:43:27, alexmos wrote: > Maybe it's enough to have coverage for lines 252-256 in one of the tests? It > might be a bit redundant in all tests. Just making sure you saw this -- though it's also minor and ok to ignore. :) https://codereview.chromium.org/1804023002/diff/500001/chrome/browser/apps/gu... File chrome/browser/apps/guest_view/web_view_browsertest.cc (right): https://codereview.chromium.org/1804023002/diff/500001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_browsertest.cc:2445: // http://crbug.com/xxxxxx <-- Don't land without filing this bug. Bug numbers still needed here. :) (Also the one in FrameView::setFrameRect and CrossProcessFrameConnector::OnFrameRectChanged) https://codereview.chromium.org/1804023002/diff/500001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/1804023002/diff/500001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:1069: -1, MSG_ROUTING_NONE, -1, FrameReplicationState(), false); nit: no change here other than wrapping, here and below, so no need to have this file in the CL. https://codereview.chromium.org/1804023002/diff/500001/content/browser/host_z... File content/browser/host_zoom_map_impl.cc (right): https://codereview.chromium.org/1804023002/diff/500001/content/browser/host_z... content/browser/host_zoom_map_impl.cc:329: // HostZoomMap serves. Should this comment be moved up to the "GetForWebContents(web_contents) != this" check? https://codereview.chromium.org/1804023002/diff/500001/content/browser/host_z... content/browser/host_zoom_map_impl.cc:539: // HostZoomMap. Should this comment also be on the GetForWebContents check? https://codereview.chromium.org/1804023002/diff/500001/content/browser/iframe... File content/browser/iframe_zoom_browsertest.cc (right): https://codereview.chromium.org/1804023002/diff/500001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:40: // subframes both (1) a change in their frame rect, and (2) a change in their nit: "have" before both? (seems like a word is missing somewhere here.) https://codereview.chromium.org/1804023002/diff/500001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:258: #if !defined(OS_ANDROID) So does this mean that on Android, despite having no page zoom UI, you can set zoom for a host and that gets correctly applied to current pages (since the first test passes) but not for future navigations to that host? And default zoom also works? I know you were going to try some of this out in a simulator, so just curious if that's what you found. https://codereview.chromium.org/1804023002/diff/500001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:261: TestNavigationObserver observer(web_contents()); NavigateToURL uses a TestNavigationObserver internally as well, and it also internally verifies the last committed URL, as long as you put EXPECT_TRUE around it, which you did. So although the extra observer doens't hurt, I don't see much benefit to it -- any specific reason for adding it? https://codereview.chromium.org/1804023002/diff/500001/content/browser/render... File content/browser/renderer_host/render_view_host_delegate.h (right): https://codereview.chromium.org/1804023002/diff/500001/content/browser/render... content/browser/renderer_host/render_view_host_delegate.h:225: // page.. nit: extra . https://codereview.chromium.org/1804023002/diff/500001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1804023002/diff/500001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4069: GURL url = pending_entry->GetURL(); What prompted adding this pending logic? Presumably all tests passed without it in PS20?
I've addressed the latest round of comments, PTAL? https://codereview.chromium.org/1804023002/diff/500001/chrome/browser/apps/gu... File chrome/browser/apps/guest_view/web_view_browsertest.cc (right): https://codereview.chromium.org/1804023002/diff/500001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_browsertest.cc:2445: // http://crbug.com/xxxxxx <-- Don't land without filing this bug. On 2016/04/27 at 23:39:51, alexmos wrote: > Bug numbers still needed here. :) (Also the one in FrameView::setFrameRect and CrossProcessFrameConnector::OnFrameRectChanged) I generally put these in just before landing, as I dislike filing bugs far in advance for issues that don't exist until the CL lands. I haven't forgotten these though. :-) https://codereview.chromium.org/1804023002/diff/500001/content/browser/frame_... File content/browser/frame_host/render_frame_host_manager_unittest.cc (right): https://codereview.chromium.org/1804023002/diff/500001/content/browser/frame_... content/browser/frame_host/render_frame_host_manager_unittest.cc:1069: -1, MSG_ROUTING_NONE, -1, FrameReplicationState(), false); On 2016/04/27 at 23:39:51, alexmos wrote: > nit: no change here other than wrapping, here and below, so no need to have this file in the CL. Indeed. I don't even remember at what stage this file got touched. Done. https://codereview.chromium.org/1804023002/diff/500001/content/browser/host_z... File content/browser/host_zoom_map_impl.cc (right): https://codereview.chromium.org/1804023002/diff/500001/content/browser/host_z... content/browser/host_zoom_map_impl.cc:329: // HostZoomMap serves. On 2016/04/27 at 23:39:51, alexmos wrote: > Should this comment be moved up to the "GetForWebContents(web_contents) != this" check? Done. https://codereview.chromium.org/1804023002/diff/500001/content/browser/host_z... content/browser/host_zoom_map_impl.cc:539: // HostZoomMap. On 2016/04/27 at 23:39:51, alexmos wrote: > Should this comment also be on the GetForWebContents check? Done. https://codereview.chromium.org/1804023002/diff/500001/content/browser/iframe... File content/browser/iframe_zoom_browsertest.cc (right): https://codereview.chromium.org/1804023002/diff/500001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:40: // subframes both (1) a change in their frame rect, and (2) a change in their On 2016/04/27 at 23:39:51, alexmos wrote: > nit: "have" before both? (seems like a word is missing somewhere here.) Done. https://codereview.chromium.org/1804023002/diff/500001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:258: #if !defined(OS_ANDROID) On 2016/04/27 at 23:39:51, alexmos wrote: > So does this mean that on Android, despite having no page zoom UI, you can set zoom for a host and that gets correctly applied to current pages (since the first test passes) but not for future navigations to that host? And default zoom also works? I know you were going to try some of this out in a simulator, so just curious if that's what you found. I didn't end up trying it in the simulator, but yes, that is what appears to happen. It's rather odd, but is a legacy behaviour. Since there's no UI to change page zoom, these tests are exercising code that never runs in Android (although in practice it could). Since the initial navigation case has received special attention in the past, I'm going to proceed with it separately to see what's going on. https://codereview.chromium.org/1804023002/diff/500001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:261: TestNavigationObserver observer(web_contents()); On 2016/04/27 at 23:39:51, alexmos wrote: > NavigateToURL uses a TestNavigationObserver internally as well, and it also internally verifies the last committed URL, as long as you put EXPECT_TRUE around it, which you did. So although the extra observer doens't hurt, I don't see much benefit to it -- any specific reason for adding it? I guess I didn't realize NavigateToURL had one internally, so no. I originally added this thinking that perhaps the Android failure was due to the page not being fully navigated when we tested the zoom level, then when it turned out not to be the problem, I left this in thinking it might be useful anyways. Removed. https://codereview.chromium.org/1804023002/diff/500001/content/browser/render... File content/browser/renderer_host/render_view_host_delegate.h (right): https://codereview.chromium.org/1804023002/diff/500001/content/browser/render... content/browser/renderer_host/render_view_host_delegate.h:225: // page.. On 2016/04/27 at 23:39:51, alexmos wrote: > nit: extra . Done. https://codereview.chromium.org/1804023002/diff/500001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1804023002/diff/500001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4069: GURL url = pending_entry->GetURL(); On 2016/04/27 at 23:39:51, alexmos wrote: > What prompted adding this pending logic? Presumably all tests passed without it in PS20? They did, but while debugging the Android test issue, I realized that the value we were passing to ViewMsg_New was invariably for the existing navigation entry, and it never considered if a navigation was pending. The tests still worked as ViewMsg_SetZoomLevelForLoadingURL is still being sent, but I would like to try and remove that pathway soon if possible. At any rate, it makes more sense that if we're creating a new view, and a navigation is pending, we would want to set the zoom for that view? But perhaps I'm missing something.
Great, thanks! LGTM once Ken's frame walking question is addressed. Also a sanity check below. https://codereview.chromium.org/1804023002/diff/500001/content/browser/iframe... File content/browser/iframe_zoom_browsertest.cc (right): https://codereview.chromium.org/1804023002/diff/500001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:258: #if !defined(OS_ANDROID) On 2016/04/28 13:20:11, wjmaclean wrote: > On 2016/04/27 at 23:39:51, alexmos wrote: > > So does this mean that on Android, despite having no page zoom UI, you can set > zoom for a host and that gets correctly applied to current pages (since the > first test passes) but not for future navigations to that host? And default > zoom also works? I know you were going to try some of this out in a simulator, > so just curious if that's what you found. > > I didn't end up trying it in the simulator, but yes, that is what appears to > happen. It's rather odd, but is a legacy behaviour. Since there's no UI to > change page zoom, these tests are exercising code that never runs in Android > (although in practice it could). Since the initial navigation case has received > special attention in the past, I'm going to proceed with it separately to see > what's going on. Acknowledged. https://codereview.chromium.org/1804023002/diff/500001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1804023002/diff/500001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4069: GURL url = pending_entry->GetURL(); On 2016/04/28 13:20:11, wjmaclean wrote: > On 2016/04/27 at 23:39:51, alexmos wrote: > > What prompted adding this pending logic? Presumably all tests passed without > it in PS20? > > They did, but while debugging the Android test issue, I realized that the value > we were passing to ViewMsg_New was invariably for the existing navigation entry, > and it never considered if a navigation was pending. The tests still worked as > ViewMsg_SetZoomLevelForLoadingURL is still being sent, but I would like to try > and remove that pathway soon if possible. > > At any rate, it makes more sense that if we're creating a new view, and a > navigation is pending, we would want to set the zoom for that view? But perhaps > I'm missing something. I think this makes sense. Just to double-check one detail: when there's a pending navigation, the previous path would go through HostZoomMap::GetZoomLevel, which would first check for UsesTemporaryZoomLevel and then use the last committed entry. Now, it will go through GetZoomLevelForHostAndScheme, where there's no temporary zoom check. Is this ok?
lgtm https://codereview.chromium.org/1804023002/diff/520001/content/browser/frame_... File content/browser/frame_host/cross_process_frame_connector.cc (right): https://codereview.chromium.org/1804023002/diff/520001/content/browser/frame_... content/browser/frame_host/cross_process_frame_connector.cc:206: // http://crbug.com/xxxxxx <- Don't land without filing this bug. Don't forget this. https://codereview.chromium.org/1804023002/diff/520001/content/browser/iframe... File content/browser/iframe_zoom_browsertest.cc (right): https://codereview.chromium.org/1804023002/diff/520001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:161: } // namespace anonymous } // namespace (no 'anonymous') https://google.github.io/styleguide/cppguide.html#Namespace_Formatting
https://codereview.chromium.org/1804023002/diff/500001/content/browser/web_co... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1804023002/diff/500001/content/browser/web_co... content/browser/web_contents/web_contents_impl.cc:4069: GURL url = pending_entry->GetURL(); On 2016/04/28 at 17:32:12, alexmos wrote: > I think this makes sense. Just to double-check one detail: when there's a pending navigation, the previous path would go through HostZoomMap::GetZoomLevel, which would first check for UsesTemporaryZoomLevel and then use the last committed entry. Now, it will go through GetZoomLevelForHostAndScheme, where there's no temporary zoom check. Is this ok? I believe that render views lose their 'temporary' status when navigating to a new domain. My assumption here is that, if we're creating a new view, we're also navigating to a 'new' domain. The old pathway isn't distinguishing between whether or not the view is newly created, it's just operating off of a request url it has received, so it can't make this distinction. https://codereview.chromium.org/1804023002/diff/520001/content/browser/frame_... File content/browser/frame_host/cross_process_frame_connector.cc (right): https://codereview.chromium.org/1804023002/diff/520001/content/browser/frame_... content/browser/frame_host/cross_process_frame_connector.cc:206: // http://crbug.com/xxxxxx <- Don't land without filing this bug. On 2016/04/28 at 22:30:14, ncarter wrote: > Don't forget this. As promised earlier, once I've collected all the necessary LGTMs I will be sure to file the bugs and insert the numbers before landing. https://codereview.chromium.org/1804023002/diff/520001/content/browser/iframe... File content/browser/iframe_zoom_browsertest.cc (right): https://codereview.chromium.org/1804023002/diff/520001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:161: } // namespace anonymous On 2016/04/28 at 22:30:14, ncarter wrote: > } // namespace > > (no 'anonymous') https://google.github.io/styleguide/cppguide.html#Namespace_Formatting Actually, is that right? The link shows a formatting rule about namespaces in general, and while it uses an example of an anonymous namespace with a comment at the end that does not include the word 'anonymous', I don't think it is prohibiting it. A git-gs search of the codebase shows numerous instances where it is used, and I think it's a useful piece of information for the reader to have without having to scroll all the way back to the top of the namespace. I'd prefer to leave it if that's ok.
Lei, while Ken and I are figuring out the frame-walk list, can you please take a look?
lgtm, since we are working on my remaining concern in another CL.
lgtm https://codereview.chromium.org/1804023002/diff/520001/chrome/browser/apps/gu... File chrome/browser/apps/guest_view/web_view_browsertest.cc (right): https://codereview.chromium.org/1804023002/diff/520001/chrome/browser/apps/gu... chrome/browser/apps/guest_view/web_view_browsertest.cc:2445: // http://crbug.com/xxxxxx <-- Don't land without filing this bug. Time to file a bug since you are starting to get LGs?
On 2016/04/29 18:03:55, Lei Zhang wrote: > lgtm > > https://codereview.chromium.org/1804023002/diff/520001/chrome/browser/apps/gu... > File chrome/browser/apps/guest_view/web_view_browsertest.cc (right): > > https://codereview.chromium.org/1804023002/diff/520001/chrome/browser/apps/gu... > chrome/browser/apps/guest_view/web_view_browsertest.cc:2445: // > http://crbug.com/xxxxxx <-- Don't land without filing this bug. > Time to file a bug since you are starting to get LGs? Yes, I'll file the bugs now, and update (and maybe rebase) the CL before landing.
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/1804023002/540001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804023002/540001
https://codereview.chromium.org/1804023002/diff/520001/content/browser/iframe... File content/browser/iframe_zoom_browsertest.cc (right): https://codereview.chromium.org/1804023002/diff/520001/content/browser/iframe... content/browser/iframe_zoom_browsertest.cc:161: } // namespace anonymous On 2016/04/29 13:40:41, wjmaclean wrote: > On 2016/04/28 at 22:30:14, ncarter wrote: > > } // namespace > > > > (no 'anonymous') > https://google.github.io/styleguide/cppguide.html#Namespace_Formatting > > Actually, is that right? The link shows a formatting rule about namespaces in > general, and while it uses an example of an anonymous namespace with a comment > at the end that does not include the word 'anonymous', I don't think it is > prohibiting it. A git-gs search of the codebase shows numerous instances where > it is used, and I think it's a useful piece of information for the reader to > have without having to scroll all the way back to the top of the namespace. > > I'd prefer to leave it if that's ok. Because over the over-arching rule about "be consistent", I generally interpret the examples in the style guide as being normative. There are 56 examples in the codebase where we do "namespace anonymous", versus 12,306 examples where we do it the way I'm advocating: https://goo.gl/bqQtk2
On 2016/04/29 19:01:56, ncarter wrote: > https://codereview.chromium.org/1804023002/diff/520001/content/browser/iframe... > File content/browser/iframe_zoom_browsertest.cc (right): > > https://codereview.chromium.org/1804023002/diff/520001/content/browser/iframe... > content/browser/iframe_zoom_browsertest.cc:161: } // namespace anonymous > On 2016/04/29 13:40:41, wjmaclean wrote: > > On 2016/04/28 at 22:30:14, ncarter wrote: > > > } // namespace > > > > > > (no 'anonymous') > > https://google.github.io/styleguide/cppguide.html#Namespace_Formatting > > > > Actually, is that right? The link shows a formatting rule about namespaces in > > general, and while it uses an example of an anonymous namespace with a comment > > at the end that does not include the word 'anonymous', I don't think it is > > prohibiting it. A git-gs search of the codebase shows numerous instances where > > it is used, and I think it's a useful piece of information for the reader to > > have without having to scroll all the way back to the top of the namespace. > > > > I'd prefer to leave it if that's ok. > > Because over the over-arching rule about "be consistent", I generally interpret > the examples in the style guide as being normative. > > There are 56 examples in the codebase where we do "namespace anonymous", versus > 12,306 examples where we do it the way I'm advocating: https://goo.gl/bqQtk2 Understood. I'll do what you want then.
The CQ bit was unchecked by wjmaclean@chromium.org
The CQ bit was checked by wjmaclean@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org, kenrb@chromium.org, nick@chromium.org, alexmos@chromium.org, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/1804023002/#ps560001 (title: "Remove 'anonymous'.")
The CQ bit was unchecked by wjmaclean@chromium.org
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
The CQ bit was unchecked by wjmaclean@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1804023002/560001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804023002/560001
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/1804023002/560001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804023002/560001
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/1804023002/560001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1804023002/560001
Message was sent while issue was closed.
Description was changed from ========== Fix page zoom to be frame-centric for out-of-process frames. This CL modifies the current methods for setting page zoom from the browser to make them work properly with out-of-process frames. HostZoomMapImpl now routes all zoom-level changes through the WebContents as the latter knows about all the frames associated with the page, regardless of which process/view they are in. This CL also modifies how default page zoom is applied, by having HostZoomMapImpl push the changes through the WebContents instead of propagating them via the RendererPrefs. This should make the code around default zoom levels easier to understand as well. It also avoids changing zoom levels on frames/views embedded in a WebContents with a custom (non-default) zoom level. BUG=528407 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix page zoom to be frame-centric for out-of-process frames. This CL modifies the current methods for setting page zoom from the browser to make them work properly with out-of-process frames. HostZoomMapImpl now routes all zoom-level changes through the WebContents as the latter knows about all the frames associated with the page, regardless of which process/view they are in. This CL also modifies how default page zoom is applied, by having HostZoomMapImpl push the changes through the WebContents instead of propagating them via the RendererPrefs. This should make the code around default zoom levels easier to understand as well. It also avoids changing zoom levels on frames/views embedded in a WebContents with a custom (non-default) zoom level. BUG=528407 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #25 (id:560001)
Message was sent while issue was closed.
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
Message was sent while issue was closed.
https://codereview.chromium.org/1804023002/diff/560001/third_party/WebKit/Sou... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1804023002/diff/560001/third_party/WebKit/Sou... third_party/WebKit/Source/web/WebViewImpl.cpp:3077: page()->setDeviceScaleFactor(m_zoomFactorForDeviceScaleFactor / m_compositorDeviceScaleFactorOverride); Why do we call the Page methods for each invocation of propagateZoomToLocalFrameRoots()? Shouldn't that be a one-time top-level calculation? The only thing I would expect to be invoked here is Frame::setPageZoomFactor. In addition, does it make sense for zoom factor to be set on a per-frame basis in Blink? Maybe it should be per Page, with a didChangeZoomFactor() call if some state on LocalFrame needs to be updated?
Message was sent while issue was closed.
Patchset 25 (id:??) landed as https://crrev.com/64951906c6fb59b33a63313f98819bbafe9eebe2 Cr-Commit-Position: refs/heads/master@{#390755}
Message was sent while issue was closed.
Description was changed from ========== Fix page zoom to be frame-centric for out-of-process frames. This CL modifies the current methods for setting page zoom from the browser to make them work properly with out-of-process frames. HostZoomMapImpl now routes all zoom-level changes through the WebContents as the latter knows about all the frames associated with the page, regardless of which process/view they are in. This CL also modifies how default page zoom is applied, by having HostZoomMapImpl push the changes through the WebContents instead of propagating them via the RendererPrefs. This should make the code around default zoom levels easier to understand as well. It also avoids changing zoom levels on frames/views embedded in a WebContents with a custom (non-default) zoom level. BUG=528407 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation ========== to ========== Fix page zoom to be frame-centric for out-of-process frames. This CL modifies the current methods for setting page zoom from the browser to make them work properly with out-of-process frames. HostZoomMapImpl now routes all zoom-level changes through the WebContents as the latter knows about all the frames associated with the page, regardless of which process/view they are in. This CL also modifies how default page zoom is applied, by having HostZoomMapImpl push the changes through the WebContents instead of propagating them via the RendererPrefs. This should make the code around default zoom levels easier to understand as well. It also avoids changing zoom levels on frames/views embedded in a WebContents with a custom (non-default) zoom level. BUG=528407 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/64951906c6fb59b33a63313f98819bbafe9eebe2 Cr-Commit-Position: refs/heads/master@{#390755} ==========
Message was sent while issue was closed.
On 2016/04/29 22:49:15, dcheng wrote: > https://codereview.chromium.org/1804023002/diff/560001/third_party/WebKit/Sou... > File third_party/WebKit/Source/web/WebViewImpl.cpp (right): > > https://codereview.chromium.org/1804023002/diff/560001/third_party/WebKit/Sou... > third_party/WebKit/Source/web/WebViewImpl.cpp:3077: > page()->setDeviceScaleFactor(m_zoomFactorForDeviceScaleFactor / > m_compositorDeviceScaleFactorOverride); > Why do we call the Page methods for each invocation of > propagateZoomToLocalFrameRoots()? Shouldn't that be a one-time top-level > calculation? The only thing I would expect to be invoked here is > Frame::setPageZoomFactor. I think our assumption was that setPageZoomFactor would only be applying the zoom factor to the locally-rooted subtree (see alexmos@'s comments on Patchset 12 in WebViewImpl), so if there were multiple locally rooted trees, we would need to set it for each of them. But I don't claim to be an expert at how zoom works once it's past WebViewImpl, so perhaps I'm wrong in thinking this. If that is the case, the current code is at least just redundant, and it should be easy to fix. I'll take a look at this today to see how it works. > In addition, does it make sense for zoom factor to be set on a per-frame basis > in Blink? Maybe it should be per Page, with a didChangeZoomFactor() call if some > state on LocalFrame needs to be updated? It's entirely possible that it would make sense to do some refactoring of how zoom is handled in Blink, but for the purposes of this CL I wanted to do minimal refactoring, on the assumption that such refactoring would be better off in a separate, smaller
Message was sent while issue was closed.
On 2016/05/02 15:30:21, wjmaclean wrote: > > It's entirely possible that it would make sense to do some refactoring of how > zoom is handled in Blink, but for the purposes of this CL I wanted to do minimal > refactoring, on the assumption that such refactoring would be better off in a > separate, smaller ... CL.
Message was sent while issue was closed.
On 2016/05/02 15:32:43, wjmaclean wrote: > On 2016/05/02 15:30:21, wjmaclean wrote: > > > > It's entirely possible that it would make sense to do some refactoring of how > > zoom is handled in Blink, but for the purposes of this CL I wanted to do > minimal > > refactoring, on the assumption that such refactoring would be better off in a > > separate, smaller > > ... CL. Having now looked at https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... it seems like (1) each LocalFrame has it's own copy of the page zoom in m_pageZoomFactor, and (2) it only propagates this information to local children. So it appears that alexmos@'s concern about A-B-A requires we visit all the local roots individually.
Message was sent while issue was closed.
On 2016/05/02 at 15:38:00, wjmaclean wrote: > On 2016/05/02 15:32:43, wjmaclean wrote: > > On 2016/05/02 15:30:21, wjmaclean wrote: > > > > > > It's entirely possible that it would make sense to do some refactoring of how > > > zoom is handled in Blink, but for the purposes of this CL I wanted to do > > minimal > > > refactoring, on the assumption that such refactoring would be better off in a > > > separate, smaller > > > > ... CL. > > Having now looked at > > https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit... > > it seems like > > (1) each LocalFrame has it's own copy of the page zoom in m_pageZoomFactor, and > (2) it only propagates this information to local children. > > So it appears that alexmos@'s concern about A-B-A requires we visit all the local roots individually. Right, we still have to notify the local roots somehow, but can you actually have a different zoom factor for different frames in the same page? Also, my comment about Page methods is independent of what we want to do about LocalFrame::setPageZoomFactor(): we pass no frame-specific information into those calls, so it never makes sense to (potentially) call them multiple times when we recurse in WebViewImpl::propagateZoomToLocalFrameRoots.
Message was sent while issue was closed.
On 2016/05/02 17:32:52, dcheng wrote: > > Right, we still have to notify the local roots somehow, but can you actually > have a different zoom factor for different frames in the same page? Although the current frames implementation would make it seem like different frames can have different zoom levels, I don't know of any use case that actually does this, nor can I think why we would want to offer it. Caveat: there may be subtleties in Blink that I am unaware of in this regard ... NB: In WebView, it *is* possible for the guest/child to have a different page zoom than it's embedder/parent. However, the child's content lives in a separate frame tree iirc, so this should ultimately be an unrelated case. So if you're thinking this state should be moved out of LocalFrame, I would tend to agree. > Also, my comment about Page methods is independent of what we want to do about > LocalFrame::setPageZoomFactor(): we pass no frame-specific information into > those calls, so it never makes sense to (potentially) call them multiple times > when we recurse in WebViewImpl::propagateZoomToLocalFrameRoots. Correct. At present the information being passed is purely on the Page (dare I say 'View'?) level, and ideally a single call would suffice. This state could be stored in the RenderView, and frames could get it from there. That being said, I know we're trying to shrink RenderView, so I'll leave it to others to suggest what the ultimate, best home might be for this state.
Message was sent while issue was closed.
On 2016/05/02 at 17:41:41, wjmaclean wrote: > On 2016/05/02 17:32:52, dcheng wrote: > > > > Right, we still have to notify the local roots somehow, but can you actually > > have a different zoom factor for different frames in the same page? > > Although the current frames implementation would make it seem like different frames can have different zoom levels, I don't know of any use case that actually does this, nor can I think why we would want to offer it. Caveat: there may be subtleties in Blink that I am unaware of in this regard ... > > NB: In WebView, it *is* possible for the guest/child to have a different page zoom than it's embedder/parent. However, the child's content lives in a separate frame tree iirc, so this should ultimately be an unrelated case. > > So if you're thinking this state should be moved out of LocalFrame, I would tend to agree. > > > Also, my comment about Page methods is independent of what we want to do about > > LocalFrame::setPageZoomFactor(): we pass no frame-specific information into > > those calls, so it never makes sense to (potentially) call them multiple times > > when we recurse in WebViewImpl::propagateZoomToLocalFrameRoots. > > Correct. At present the information being passed is purely on the Page (dare I say 'View'?) level, and ideally a single call would suffice. This state could be stored in the RenderView, and frames could get it from there. That being said, I know we're trying to shrink RenderView, so I'll leave it to others to suggest what the ultimate, best home might be for this state. We don't need to store it on RenderView: it just shouldn't be invoked in the recursive bits of this function. The Page functions should just be called once in setZoomLevel().
Message was sent while issue was closed.
It certainly does feel like it would be a lot simpler if these zoom values were on Page rather than LocalFrame. The only reason I can see is that popups don't get their own Page but apparently can have a zoom factor that differs from the real page zoom factor, which can be affected by CSS applied to the element that creates the popup.
Message was sent while issue was closed.
On 2016/05/02 19:23:02, kenrb wrote: > It certainly does feel like it would be a lot simpler if these zoom values were > on Page rather than LocalFrame. The only reason I can see is that popups don't > get their own Page but apparently can have a zoom factor that differs from the > real page zoom factor, which can be affected by CSS applied to the element that > creates the popup. Right, I guess that's what I meant when I said 'on RenderView or somewhere relevant' ... blink::Page is 'somewhere relevant' (I wasn't thinking of blink::Page when I read Daniel's comments above, rather I was thinking of Page as a concept). We already have Page::setDeviceScaleFactor() and (tangentially) Page::setPageScaleFactor(), so Page::setPageZoomFactor() or Page::setPageZoomLevel(). I'm not sure popups would prevent us from doing this; they can be given their own zoom calculated at the time they are created (disclaimer: I have no knowledge off the top of my head about their creation process, so there might be more to this than what I've suggested).
Message was sent while issue was closed.
On 2016/05/02 19:56:01, wjmaclean wrote: > On 2016/05/02 19:23:02, kenrb wrote: > > It certainly does feel like it would be a lot simpler if these zoom values > were > > on Page rather than LocalFrame. The only reason I can see is that popups don't > > get their own Page but apparently can have a zoom factor that differs from the > > real page zoom factor, which can be affected by CSS applied to the element > that > > creates the popup. > > Right, I guess that's what I meant when I said 'on RenderView or somewhere > relevant' ... blink::Page is 'somewhere relevant' (I wasn't thinking of > blink::Page when I read Daniel's comments above, rather I was thinking of Page > as a concept). We already have Page::setDeviceScaleFactor() and (tangentially) > Page::setPageScaleFactor(), so Page::setPageZoomFactor() or > Page::setPageZoomLevel(). I'm not sure popups would prevent us from doing this; > they can be given their own zoom calculated at the time they are created > (disclaimer: I have no knowledge off the top of my head about their creation > process, so there might be more to this than what I've suggested). They are already given their own zoom when they are created, which is stored in the popup's LocalFrame::m_pageZoomFactor. So simply moving m_pageZoomFactor to Page and having LocalFrame reference the Page's value rather than storing a copy seems like it would break popup zoom. You could do something like have a zoom factor override on LocalFrame that can be optionally set to tell it not to go to the Page.
Message was sent while issue was closed.
On 2016/05/02 20:13:40, kenrb wrote: > On 2016/05/02 19:56:01, wjmaclean wrote: > > On 2016/05/02 19:23:02, kenrb wrote: > > > It certainly does feel like it would be a lot simpler if these zoom values > > were > > > on Page rather than LocalFrame. The only reason I can see is that popups > don't > > > get their own Page but apparently can have a zoom factor that differs from > the > > > real page zoom factor, which can be affected by CSS applied to the element > > that > > > creates the popup. > > > > Right, I guess that's what I meant when I said 'on RenderView or somewhere > > relevant' ... blink::Page is 'somewhere relevant' (I wasn't thinking of > > blink::Page when I read Daniel's comments above, rather I was thinking of Page > > as a concept). We already have Page::setDeviceScaleFactor() and (tangentially) > > Page::setPageScaleFactor(), so Page::setPageZoomFactor() or > > Page::setPageZoomLevel(). I'm not sure popups would prevent us from doing > this; > > they can be given their own zoom calculated at the time they are created > > (disclaimer: I have no knowledge off the top of my head about their creation > > process, so there might be more to this than what I've suggested). > > They are already given their own zoom when they are created, which is stored in > the popup's LocalFrame::m_pageZoomFactor. > > So simply moving m_pageZoomFactor to Page and having LocalFrame reference the > Page's value rather than storing a copy seems like it would break popup zoom. > You could do something like have a zoom factor override on LocalFrame that can > be optionally set to tell it not to go to the Page. Ahh, ok ... I did not realize that the popup was using LocalFrame::m_pageZoomFactor to store its zoom. Would having an override be expensive, in the sense that most LocalFrames aren't serving popups? Could the zoom factor be stored directly on the popup object instead? I haven't had a chance to look at any of the popup code yet. |
