Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1143323009:
Report new screen position to renderer when ancestor window moves. (Closed)
|
Created:
5 years, 7 months ago by mharanczyk Modified:
5 years, 6 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, penghuang+watch_chromium.org, sievers+watch_chromium.org, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, kalyank, piman+watch_chromium.org, danakj+watch_chromium.org, James Su Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionReport new screen position to renderer when ancestor window moves.
This change is related to https://codereview.chromium.org/1062273002/.
Without it renderer can draw controls (dropdowns, etc.) out of place,
because it will have outdated screen coordinates.
BUG=469857, 468669
Committed: https://crrev.com/647e2a749681a5f9e5bba3510524d898f9b4b910
Cr-Commit-Position: refs/heads/master@{#333749}
Patch Set 1 #Patch Set 2 : Added unittest. #
Total comments: 9
Patch Set 3 : Address review nits. #Patch Set 4 : Rebase #Patch Set 5 : Do not try to send screen rect during shutdown. #
Messages
Total messages: 32 (11 generated)
mharanczyk@opera.com changed reviewers: + sadrul@chromium.org, sky@chromium.org
@sky, please take a look, this CL is a follow up of our mail discussion.
Can this have a test?
I haven't find a nice way to test it, I will keep looking. If you have any idea, your advice will be most welcome.
On 2015/05/27 15:17:16, mharanczyk wrote: > I haven't find a nice way to test it, I will keep looking. If you have any idea, > your advice will be most welcome. The test should be in render_widget_host_view_aura_unittest.cc: the test could create a new aura::Window as a parent to the RWHVA's window, move the parent window, and verify that the right IPC message is sent (there are several tests in there that do this for input-event related IPC messages).
On 2015/05/27 15:33:37, sadrul wrote: > The test should be in render_widget_host_view_aura_unittest.cc: the test could > create a new aura::Window as a parent to the RWHVA's window, move the parent > window, and verify that the right IPC message is sent (there are several tests > in there that do this for input-event related IPC messages). Thank you! I haven't thought about just checking IPC message.
Added unit test, could you please take another look.
Ping! Can anyone please take a look?
A couple of nits. Otherwise, lgtm https://codereview.chromium.org/1143323009/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/1143323009/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:841: EXPECT_EQ(1U, sink_->message_count()); Do an ASSERT_EQ here instead, and remove the conditional below in line 844. https://codereview.chromium.org/1143323009/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:848: EXPECT_EQ(gfx::Rect(24, 24, 100, 100), get<0>(params)); base::get<>? (everywhere else in this test too) https://codereview.chromium.org/1143323009/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:858: EXPECT_EQ(1U, sink_->message_count()); ditto
https://codereview.chromium.org/1143323009/diff/20001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura_unittest.cc (right): https://codereview.chromium.org/1143323009/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:841: EXPECT_EQ(1U, sink_->message_count()); On 2015/06/03 18:51:41, sadrul wrote: > Do an ASSERT_EQ here instead, and remove the conditional below in line 844. Done. https://codereview.chromium.org/1143323009/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:841: EXPECT_EQ(1U, sink_->message_count()); On 2015/06/03 18:51:41, sadrul wrote: > Do an ASSERT_EQ here instead, and remove the conditional below in line 844. Done. https://codereview.chromium.org/1143323009/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:848: EXPECT_EQ(gfx::Rect(24, 24, 100, 100), get<0>(params)); On 2015/06/03 18:51:41, sadrul wrote: > base::get<>? (everywhere else in this test too) get is not defined in any namespace. https://codereview.chromium.org/1143323009/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:848: EXPECT_EQ(gfx::Rect(24, 24, 100, 100), get<0>(params)); On 2015/06/03 18:51:41, sadrul wrote: > base::get<>? (everywhere else in this test too) get<> is placed outside of base namespace in base/tuple.h. https://codereview.chromium.org/1143323009/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:858: EXPECT_EQ(1U, sink_->message_count()); On 2015/06/03 18:51:41, sadrul wrote: > ditto Done. https://codereview.chromium.org/1143323009/diff/20001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura_unittest.cc:858: EXPECT_EQ(1U, sink_->message_count()); On 2015/06/03 18:51:41, sadrul wrote: > ditto Done.
The CQ bit was checked by mharanczyk@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/1143323009/#ps40001 (title: "Address review nits.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1143323009/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_gn_dbg on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Rebase moved tuple get<> to base namespace. Fixed that.
The CQ bit was checked by mharanczyk@opera.com
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/1143323009/#ps60001 (title: "Rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1143323009/60001
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by mharanczyk@opera.com to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/1143323009/#ps80001 (title: "Do not try to send screen rect during shutdown.")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1143323009/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I had to add conditional to not send rects when shutting down to avoid crashes in extension apps. @sadrul, could you please take a look at this CL again to double check if it is fine?
On 2015/06/08 15:45:29, mharanczyk wrote: > I had to add conditional to not send rects when shutting down to avoid crashes > in extension apps. > > @sadrul, could you please take a look at this CL again to double check if it is > fine? slgtm
The CQ bit was checked by mharanczyk@opera.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1143323009/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/647e2a749681a5f9e5bba3510524d898f9b4b910 Cr-Commit-Position: refs/heads/master@{#333749} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
