Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1078123002:
Send resize event when the screen info changes. (Closed)
|
Created:
5 years, 8 months ago by mlamouri (slow - plz ping) Modified:
5 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, kalyank, sadrul Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSend resize event when the screen info changes.
This is a regression from https://codereview.chromium.org/953233002
It depends on https://codereview.chromium.org/1100503003
BUG=475939
Committed: https://crrev.com/d9a98e97395e76736faab14cce3158ad295ca0f4
Cr-Commit-Position: refs/heads/master@{#329088}
Patch Set 1 #
Total comments: 5
Patch Set 2 : update #Patch Set 3 : rebase #
Total comments: 4
Patch Set 4 : comments applied #Patch Set 5 : fix tests #Patch Set 6 : fixing flakyness #Patch Set 7 : update unit tests #Patch Set 8 : reduced change #Patch Set 9 : simpler #
Dependent Patchsets: Messages
Total messages: 41 (11 generated)
mlamouri@chromium.org changed reviewers: + sadrul@chromium.org, sievers@chromium.org
sievers@, could you have a look at the changes in: content/browser/renderer_host/ sadrul@, could you have a look at the changes in: ui/aura/test/test_screen.cc
sievers@chromium.org changed reviewers: + piman@chromium.org
https://codereview.chromium.org/1078123002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1078123002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_impl.cc:588: } Should we just move this out to WasResized()? And then pass in |screen_info_was_out_of_date|. Then we can make this getter methods const without hidden side-effects.
piman@chromium.org changed reviewers: + jochen@chromium.org
https://codereview.chromium.org/1078123002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_unittest.cc (right): https://codereview.chromium.org/1078123002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_unittest.cc:611: EXPECT_TRUE(process_->sink().GetUniqueMessageMatching(ViewMsg_Resize::ID)); ClearMessages() afterwards before the next block? https://codereview.chromium.org/1078123002/diff/1/ui/aura/test/test_screen.cc File ui/aura/test/test_screen.cc (right): https://codereview.chromium.org/1078123002/diff/1/ui/aura/test/test_screen.cc... ui/aura/test/test_screen.cc:68: host_->SetRootTransform(GetRotationTransform() * GetUIScaleTransform()); We don't do null checks elsewhere in this file. Is some test missing a call to CreateHostForPrimaryDisplay()?
Comments applied. PTAL. https://codereview.chromium.org/1078123002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1078123002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_impl.cc:588: } On 2015/04/10 at 18:40:31, sievers wrote: > Should we just move this out to WasResized()? > And then pass in |screen_info_was_out_of_date|. > > Then we can make this getter methods const without hidden side-effects. Done. https://codereview.chromium.org/1078123002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_unittest.cc (right): https://codereview.chromium.org/1078123002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_unittest.cc:611: EXPECT_TRUE(process_->sink().GetUniqueMessageMatching(ViewMsg_Resize::ID)); On 2015/04/10 at 22:58:30, sadrul wrote: > ClearMessages() afterwards before the next block? Done.
jochen@chromium.org changed reviewers: + danakj@chromium.org
lgtm https://codereview.chromium.org/1078123002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1078123002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:1146: latency_tracker_.set_device_scale_factor(result->deviceScaleFactor); This would also be nice to follow up on (maybe a TODO that this method should be const?). Maybe we can just change LatencyTracker and pass the current scale factor with each OnInputEvent()
https://codereview.chromium.org/1078123002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (left): https://codereview.chromium.org/1078123002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:586: } Do we need to call UpdateScreenInfo() in RenderViewHostImpl::CreateRenderView (which also calls GetResizeParam), or is there a reason why screen_info_ is correct in that case? Can we add a DCHECK(!screen_info_out_of_date_) here?
PTAL https://codereview.chromium.org/1078123002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (left): https://codereview.chromium.org/1078123002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:586: } On 2015/04/15 at 19:38:42, piman (Very slow to review) wrote: > Do we need to call UpdateScreenInfo() in RenderViewHostImpl::CreateRenderView (which also calls GetResizeParam), or is there a reason why screen_info_ is correct in that case? > > Can we add a DCHECK(!screen_info_out_of_date_) here? Done. https://codereview.chromium.org/1078123002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/1078123002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:1146: latency_tracker_.set_device_scale_factor(result->deviceScaleFactor); On 2015/04/15 at 18:17:00, sievers wrote: > This would also be nice to follow up on (maybe a TODO that this method should be const?). > > Maybe we can just change LatencyTracker and pass the current scale factor with each OnInputEvent() TODO added.
lgtm
The CQ bit was checked by mlamouri@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org Link to the patchset: https://codereview.chromium.org/1078123002/#ps80001 (title: "fix tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1078123002/80001
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6e3582ad1a058af30aa44ca7fae2113b050aa437 Cr-Commit-Position: refs/heads/master@{#325494}
Message was sent while issue was closed.
A revert of this CL (patchset #5 id:80001) has been created in https://codereview.chromium.org/1093713002/ by maniscalco@chromium.org. The reason for reverting is: Suspected as cause of flaky failures for WebViewInteractiveTest.PopupPositioningBasic WebViewInteractiveTest.PopupPositioningMoved. For details, see https://code.google.com/p/chromium/issues/detail?id=477783 .
sievers@, PTAL. I have updated the CL to no longer send a resize event if the screen_info_ hasn't actually changed even if it was invalidated.
The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from sievers@chromium.org, piman@chromium.org Link to the patchset: https://codereview.chromium.org/1078123002/#ps100001 (title: "fixing flakyness")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1078123002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Hmm, this is getting a little confusing now between Invalidate/UpdateScreenInfo and screen_info_is_out_of_date_ and comparing the actual values. Can you explain how ps4 caused a regression?
On 2015/04/22 at 21:12:43, sievers wrote: > Hmm, this is getting a little confusing now between Invalidate/UpdateScreenInfo and screen_info_is_out_of_date_ and comparing the actual values. > Can you explain how ps4 caused a regression? There is a test with a date picker popup that became flaky because it was receiving a resize event (which hides the popup). Preventing messages to be sent when there is no real change fixes the issue. IIRC, this is coming from the fact that we are now only making the screen info as no longer out of date when the new information are sent to the renderer process. It used to be done when GetWebScreenInfo() was called which was happening outside of WasResized(). I think it would be great to land a fix sooner than later because the regression this CL is fixing is impacting M43.
On 2015/04/22 22:18:27, Mounir Lamouri wrote: > On 2015/04/22 at 21:12:43, sievers wrote: > > Hmm, this is getting a little confusing now between > Invalidate/UpdateScreenInfo and screen_info_is_out_of_date_ and comparing the > actual values. > > Can you explain how ps4 caused a regression? > > There is a test with a date picker popup that became flaky because it was > receiving a resize event (which hides the popup). Preventing messages to be sent > when there is no real change fixes the issue. IIRC, this is coming from the fact > that we are now only making the screen info as no longer out of date when the > new information are sent to the renderer process. It used to be done when > GetWebScreenInfo() was called which was happening outside of WasResized(). But the other call sites don't send the info to the renderer. And none of the call sites look like they should be expecting any side effects from that method. If it fixes the issue, maybe rather put the screen_info_is_out_of_date_-reset back into GetWebScreenInfo() for now? (And integrate it with the TODO to make that method const.) Are you sure it's not GetResizeParams() now returning |dirty| where it before wasn't? But wouldn't the DCHECK fail? Did you run the tests with DCHECKs?
On 2015/04/22 22:18:27, Mounir Lamouri wrote: > I think it would be great to land a fix sooner than later because the regression > this CL is fixing is impacting M43. Yes, but in that case I would rather suggest a more minimal change to fix the original problem, which was a simple ordering issue.
What about the new patchset? Is very basic and should be enough to fix the regression (though, might trigger the flaky test, not sure..). We can discuss a different design in a follow-up CL.
lgtm. thanks for investigating this!
Ok, since you said this needs to be reopened, since some test is still flaky, some comments: This code is ridiculously complex. Even worse, there are so many testing hooks and overrides (SetXXXForTesting, ScreenMetricsEmulator, and then all the different unittests manually setting all kinds of things and testing certain sequences), that it's hard to say what really all might be wrong. You suggested that what works is patch set 7, i.e. not sending the resize if the WebScreenInfo didn't change (and nothing else changed). But what's terrible is that we then have the need for 3 separate steps then: - InvalidateScreenInfo() to avoid redundant calls to retrieve the screen info Is this really so expensive or can we just avoid all of this? - GetWebScreenInfo() to retrieve the screen info - UpdateScreenInfo() to call the above (retrieve it), deal with whether it changed, and update the cached copy This looks like we will almost certainly get it wrong somewhere. Can we step back and define what the simplest behavior is we want? For example, if we just want to avoid redundant resize messages, can we just compare if something changed every time? Can we revisit if it's really expensive to retrieve the up-to-date screen info every time and if so, try to find the simplest workaround for that? (Maybe it only affects a certain platform, and maybe for example it can be cached at a lower level etc.)
On 2015/05/07 at 19:06:57, sievers wrote: > Ok, since you said this needs to be reopened, since some test is still flaky, some comments: > > This code is ridiculously complex. Even worse, there are so many testing hooks and overrides (SetXXXForTesting, ScreenMetricsEmulator, and then all the different unittests manually setting all kinds of things and testing certain sequences), that it's hard to say what really all might be wrong. > > You suggested that what works is patch set 7, i.e. not sending the resize if the WebScreenInfo didn't change (and nothing else changed). > > But what's terrible is that we then have the need for 3 separate steps then: > - InvalidateScreenInfo() to avoid redundant calls to retrieve the screen info > Is this really so expensive or can we just avoid all of this? > - GetWebScreenInfo() to retrieve the screen info > - UpdateScreenInfo() to call the above (retrieve it), deal with whether it changed, and update the cached copy > > This looks like we will almost certainly get it wrong somewhere. > > Can we step back and define what the simplest behavior is we want? For example, if we just want to avoid redundant resize messages, can we just compare if something changed every time? Can we revisit if it's really expensive to retrieve the up-to-date screen info every time and if so, try to find the simplest workaround for that? (Maybe it only affects a certain platform, and maybe for example it can be cached at a lower level etc.) I agree that the code is quite complicated (though, I do not think that my change makes it worse) but my goal here is to fix the regression. I think it is not part of the regression fix to refactor this code beyond what is needed. It is already unfortunate that the regression was caught too late to actually permit a sane revert.
sievers@, I tried another approach following our discussion over IM. That approach is simpler but will compute the WebScreenInfo every time. On Aura, we only read the information from the gfx::Display which is updated by some other means. On MacOS, we are calling [NSScreen deepestScreen] which shouldn't be a crazy expensive call but surely more expensive than caching. PTAL.
LGTM, this is nice and simple. Agreed, the caching of |screen_info_| shouldn't be needed anymore since the code has changed with AURA and doesn't make all sorts of X and gdk calls anymore. I have tried reproducing the failures that your original simple patch set 8 caused (WebViewInteractiveTest.PopupPositioningBasic, WebViewInteractiveTest.PopupPositioningMoved). It is not very easy to repro (maybe 1 in a 100 or so for me). I have run it with ps9 and it seems to not be flaky although hard to be fully sure with that repro rate unless you run it overnight. (It could be that there is another underlying flakiness issue in the test, for example.)
The CQ bit was checked by mlamouri@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from piman@chromium.org Link to the patchset: https://codereview.chromium.org/1078123002/#ps160001 (title: "simpler")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1078123002/160001
Message was sent while issue was closed.
Committed patchset #9 (id:160001)
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/d9a98e97395e76736faab14cce3158ad295ca0f4 Cr-Commit-Position: refs/heads/master@{#329088} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
