Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1568383002:
Preserve page resize information when main frame is remote (Closed)
DescriptionPreserve page resize information when main frame is remote
Currently, resizes calls to WebViewImpl and VisualViewport are ignored
when there is a remote main frame, since the top-level page and viewport
sizes are irrelevant to its rendering. However, this causes incorrect
rendering when a remote main frame is swapped to a local frame.
This patch allows WebViewImpl and VisualViewport to keep the sizes
set in that case, so that after a main frame swap the new local frame
can initialize with correct values and render properly.
BUG=526211, 572841
Committed: https://crrev.com/845e05d134b2bc9b6a3e360325a3c766ad2fa7ac
Cr-Commit-Position: refs/heads/master@{#368986}
Patch Set 1 #Patch Set 2 : git cl format #Patch Set 3 : Bug fix #
Total comments: 5
Patch Set 4 : dcheng's suggestion #
Total comments: 5
Patch Set 5 : bokan review comments addressed #
Total comments: 4
Patch Set 6 : More dcheng comments addressed + fixed an error in last PS #Patch Set 7 : Speculative test failure fix #Patch Set 8 : Small tweak to width comparison in setSize #
Messages
Total messages: 20 (6 generated)
Description was changed from ========== Preserve page resize information when main frame is remote Currently, resizes calls to WebViewImpl and VisualViewport are ignored when there is a remote main frame, since the top-level page and viewport sizes are irrelevant to its rendering. However, this causes incorrect rendering when a remote main frame is swapped to a local frame. This patch allows WebViewImpl and VisualViewport to keep the sizes set in that case, so that after a main frame swap the new local frame can initialize with correct values and render properly. BUG=526211, 572841 ========== to ========== Preserve page resize information when main frame is remote Currently, resizes calls to WebViewImpl and VisualViewport are ignored when there is a remote main frame, since the top-level page and viewport sizes are irrelevant to its rendering. However, this causes incorrect rendering when a remote main frame is swapped to a local frame. This patch allows WebViewImpl and VisualViewport to keep the sizes set in that case, so that after a main frame swap the new local frame can initialize with correct values and render properly. BUG=526211, 572841 ==========
kenrb@chromium.org changed reviewers: + bokan@chromium.org, dcheng@chromium.org
PTAL: dcheng@ for WebViewImpl.cpp and the test bokan@ for VisualViewport.cpp
https://codereview.chromium.org/1568383002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right): https://codereview.chromium.org/1568383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/VisualViewport.cpp:191: void VisualViewport::setScaleAndLocation(float scale, const FloatPoint& location) What actually needs to run in here for a WebView with a remote main frame? Can we do all the common work first and then early return halfway through?
https://codereview.chromium.org/1568383002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right): https://codereview.chromium.org/1568383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/VisualViewport.cpp:191: void VisualViewport::setScaleAndLocation(float scale, const FloatPoint& location) Can a remote frame even be pinch-zoomed? I think maybe we don't need this method to work for remote frames. e.g. If there's no frame than the clamping logic below doesn't work (and will zero out the offset currently). Perhaps the way this should work is that the root frame should get the pinch gesture in its local process, then send some message to child frames' processes where these values can be updated through some new method (e.g. VisualViewport::updateValuesFromRemoteRoot)
https://codereview.chromium.org/1568383002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right): https://codereview.chromium.org/1568383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/VisualViewport.cpp:191: void VisualViewport::setScaleAndLocation(float scale, const FloatPoint& location) On 2016/01/11 16:36:34, bokan wrote: > Can a remote frame even be pinch-zoomed? I think maybe we don't need this method > to work for remote frames. e.g. If there's no frame than the clamping logic > below doesn't work (and will zero out the offset currently). > > Perhaps the way this should work is that the root frame should get the pinch > gesture in its local process, then send some message to child frames' processes > where these values can be updated through some new method (e.g. > VisualViewport::updateValuesFromRemoteRoot) Pinch zoom doesn't really make sense on remote frames, so I think a lot of this can be ignored, as Daniel suggests. I uploaded a new patchset that might make more sense. I think we need to preserve the scale here, though, because if the page scale gets changed and then we swap the remote main frame back to a local main frame, it probably needs to be correct in the same way as the size. Does that sound right?
https://codereview.chromium.org/1568383002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right): https://codereview.chromium.org/1568383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/VisualViewport.cpp:191: void VisualViewport::setScaleAndLocation(float scale, const FloatPoint& location) On 2016/01/11 17:07:24, kenrb wrote: > On 2016/01/11 16:36:34, bokan wrote: > > Can a remote frame even be pinch-zoomed? I think maybe we don't need this > method > > to work for remote frames. e.g. If there's no frame than the clamping logic > > below doesn't work (and will zero out the offset currently). > > > > Perhaps the way this should work is that the root frame should get the pinch > > gesture in its local process, then send some message to child frames' > processes > > where these values can be updated through some new method (e.g. > > VisualViewport::updateValuesFromRemoteRoot) > > Pinch zoom doesn't really make sense on remote frames, so I think a lot of this > can be ignored, as Daniel suggests. I uploaded a new patchset that might make > more sense. > > I think we need to preserve the scale here, though, because if the page scale > gets changed and then we swap the remote main frame back to a local main frame, > it probably needs to be correct in the same way as the size. Does that sound > right? It sounds fine in principle but practically speaking, the only time we should be swapping from remote to local is on a navigation right? That should always reset the scale anyway. My point earlier was that I don't think we should be changing the scale from a remote frames' process at all. Pinch gestures should target the root frame and other actions that cause zooming (such as focusing a text box on mobile) would make more sense to send a message to the root and let it do the zoom. In that case, we wouldn't have to worry about the VisualViewport's scale/location changing in the remote process. Anyway, I know this is all in flight so I'll stamp this change since it moves us along. Just putting in my 2c for future direction :). https://codereview.chromium.org/1568383002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1568383002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebViewImpl.cpp:1931: pageScaleConstraintsSet().didChangeViewSize(m_size); Is this call to didChangeViewSize actually needed to make your case work? pageScaleConstraintsSet is responsible for calculating min/max page scales and layout width. I doubt it'll work as expected since it also needs the content width which a remote process wont have, so unless you needed this I'd remove it. https://codereview.chromium.org/1568383002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/1568383002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:7039: WebFloatSize floatSize = localFrame->view()->visualViewportSize(); This actually returns the scaled size of the viewport. It works here since you don't change the scale but I think it'd be more correct to get a ref to the VisualViewport and test it's size() method explicitly. (e.g. depending on the loaded page and how much initialization we go through here, on Android this will fail since it initializes to minimum scale).
New patchset up with comments addressed. https://codereview.chromium.org/1568383002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right): https://codereview.chromium.org/1568383002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/VisualViewport.cpp:191: void VisualViewport::setScaleAndLocation(float scale, const FloatPoint& location) On 2016/01/11 18:18:58, bokan wrote: > On 2016/01/11 17:07:24, kenrb wrote: > > On 2016/01/11 16:36:34, bokan wrote: > > > Can a remote frame even be pinch-zoomed? I think maybe we don't need this > > method > > > to work for remote frames. e.g. If there's no frame than the clamping logic > > > below doesn't work (and will zero out the offset currently). > > > > > > Perhaps the way this should work is that the root frame should get the pinch > > > gesture in its local process, then send some message to child frames' > > processes > > > where these values can be updated through some new method (e.g. > > > VisualViewport::updateValuesFromRemoteRoot) > > > > Pinch zoom doesn't really make sense on remote frames, so I think a lot of > this > > can be ignored, as Daniel suggests. I uploaded a new patchset that might make > > more sense. > > > > I think we need to preserve the scale here, though, because if the page scale > > gets changed and then we swap the remote main frame back to a local main > frame, > > it probably needs to be correct in the same way as the size. Does that sound > > right? > > It sounds fine in principle but practically speaking, the only time we should be > swapping from remote to local is on a navigation right? That should always reset > the scale anyway. > > My point earlier was that I don't think we should be changing the scale from a > remote frames' process at all. Pinch gestures should target the root frame and > other actions that cause zooming (such as focusing a text box on mobile) would > make more sense to send a message to the root and let it do the zoom. In that > case, we wouldn't have to worry about the VisualViewport's scale/location > changing in the remote process. > > Anyway, I know this is all in flight so I'll stamp this change since it moves us > along. Just putting in my 2c for future direction :). I've removed this in the most recent patchset, since it isn't necessary to fix current bugs and was speculative, that having stale values on VisualViewport when a page is navigated in might be a problem in the same way as having a stale size. Your comment makes it sounds like that is not likely to be possible, so it is probably best to leave the method as is and investigate if we see any related bugs pop up later on. https://codereview.chromium.org/1568383002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1568383002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebViewImpl.cpp:1931: pageScaleConstraintsSet().didChangeViewSize(m_size); On 2016/01/11 18:18:58, bokan wrote: > Is this call to didChangeViewSize actually needed to make your case work? > pageScaleConstraintsSet is responsible for calculating min/max page scales and > layout width. I doubt it'll work as expected since it also needs the content > width which a remote process wont have, so unless you needed this I'd remove it. The original test case on https://crbug.com/526211 results in a page being rendered with incorrect size without this line. When we are swapping from remote to a local main frame we don't necessarily get a resize after the swap has completed, so not updating the pageScaleConstraintsSet on the resize that we get before the frame swap still affects rendering. https://codereview.chromium.org/1568383002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/1568383002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/tests/WebFrameTest.cpp:7039: WebFloatSize floatSize = localFrame->view()->visualViewportSize(); On 2016/01/11 18:18:58, bokan wrote: > This actually returns the scaled size of the viewport. It works here since you > don't change the scale but I think it'd be more correct to get a ref to the > VisualViewport and test it's size() method explicitly. (e.g. depending on the > loaded page and how much initialization we go through here, on Android this will > fail since it initializes to minimum scale). Done.
lgtm https://codereview.chromium.org/1568383002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1568383002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebViewImpl.cpp:1931: pageScaleConstraintsSet().didChangeViewSize(m_size); On 2016/01/11 20:50:26, kenrb wrote: > On 2016/01/11 18:18:58, bokan wrote: > > Is this call to didChangeViewSize actually needed to make your case work? > > pageScaleConstraintsSet is responsible for calculating min/max page scales and > > layout width. I doubt it'll work as expected since it also needs the content > > width which a remote process wont have, so unless you needed this I'd remove > it. > > The original test case on https://crbug.com/526211 results in a page being > rendered with incorrect size without this line. When we are swapping from remote > to a local main frame we don't necessarily get a resize after the swap has > completed, so not updating the pageScaleConstraintsSet on the resize that we get > before the frame swap still affects rendering. Ok, sgtm
https://codereview.chromium.org/1568383002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right): https://codereview.chromium.org/1568383002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/VisualViewport.cpp:95: bool autosizerNeedsUpdating = (size.width() != m_size.width()) Move the original mainFrame() check down to line 109, and this line after it. Then we can have a clearer comment about the skipped code for remote frames. https://codereview.chromium.org/1568383002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1568383002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebViewImpl.cpp:1932: page()->frameHost().visualViewport().setSize(m_size); Can we have a followup patch to better consolidate this logic? It seems really unfortunately to have to duplicate part of performResize() (which is indirectly called for the local frame path) in here.
https://codereview.chromium.org/1568383002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/VisualViewport.cpp (right): https://codereview.chromium.org/1568383002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/VisualViewport.cpp:95: bool autosizerNeedsUpdating = (size.width() != m_size.width()) On 2016/01/11 21:45:30, dcheng wrote: > Move the original mainFrame() check down to line 109, and this line after it. > Then we can have a clearer comment about the skipped code for remote frames. Done. https://codereview.chromium.org/1568383002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/1568383002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/web/WebViewImpl.cpp:1932: page()->frameHost().visualViewport().setSize(m_size); On 2016/01/11 21:45:30, dcheng wrote: > Can we have a followup patch to better consolidate this logic? It seems really > unfortunately to have to duplicate part of performResize() (which is indirectly > called for the local frame path) in here. Filed as https://code.google.com/p/chromium/issues/detail?id=576401
lgtm
lgtm
The CQ bit was checked by kenrb@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org Link to the patchset: https://codereview.chromium.org/1568383002/#ps140001 (title: "Small tweak to width comparison in setSize")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1568383002/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1568383002/140001
Message was sent while issue was closed.
Description was changed from ========== Preserve page resize information when main frame is remote Currently, resizes calls to WebViewImpl and VisualViewport are ignored when there is a remote main frame, since the top-level page and viewport sizes are irrelevant to its rendering. However, this causes incorrect rendering when a remote main frame is swapped to a local frame. This patch allows WebViewImpl and VisualViewport to keep the sizes set in that case, so that after a main frame swap the new local frame can initialize with correct values and render properly. BUG=526211, 572841 ========== to ========== Preserve page resize information when main frame is remote Currently, resizes calls to WebViewImpl and VisualViewport are ignored when there is a remote main frame, since the top-level page and viewport sizes are irrelevant to its rendering. However, this causes incorrect rendering when a remote main frame is swapped to a local frame. This patch allows WebViewImpl and VisualViewport to keep the sizes set in that case, so that after a main frame swap the new local frame can initialize with correct values and render properly. BUG=526211, 572841 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Preserve page resize information when main frame is remote Currently, resizes calls to WebViewImpl and VisualViewport are ignored when there is a remote main frame, since the top-level page and viewport sizes are irrelevant to its rendering. However, this causes incorrect rendering when a remote main frame is swapped to a local frame. This patch allows WebViewImpl and VisualViewport to keep the sizes set in that case, so that after a main frame swap the new local frame can initialize with correct values and render properly. BUG=526211, 572841 ========== to ========== Preserve page resize information when main frame is remote Currently, resizes calls to WebViewImpl and VisualViewport are ignored when there is a remote main frame, since the top-level page and viewport sizes are irrelevant to its rendering. However, this causes incorrect rendering when a remote main frame is swapped to a local frame. This patch allows WebViewImpl and VisualViewport to keep the sizes set in that case, so that after a main frame swap the new local frame can initialize with correct values and render properly. BUG=526211, 572841 Committed: https://crrev.com/845e05d134b2bc9b6a3e360325a3c766ad2fa7ac Cr-Commit-Position: refs/heads/master@{#368986} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/845e05d134b2bc9b6a3e360325a3c766ad2fa7ac Cr-Commit-Position: refs/heads/master@{#368986} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
