Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1958903005:
Fix positioning of Browser Plugins (Closed)
|
Created:
4 years, 7 months ago by Stephen Chennney Modified:
4 years, 7 months ago CC:
blink-reviews, chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix positioning of widgets
The BrowserPlugin updateGeometry code was only sending a UpdateGeometry message to its host if the rect was the _same_ as the previous rect. This required two calls to the method in order to get the message sent, and no doubt resulted in the message being sent every time the geometry was the same and never when it was different.
R=fsamuel@chromium.org, lazyboy@chromium.org
BUG=555201, 596494
Committed: https://crrev.com/b999309cd9ace1b703df73ed50d1c0d9efde90fa
Cr-Commit-Position: refs/heads/master@{#394808}
Patch Set 1 #Patch Set 2 : Fix underlying cause #Patch Set 3 : Fix positioning problem and update test #
Total comments: 4
Patch Set 4 : Update less #
Messages
Total messages: 45 (15 generated)
The CQ bit was checked by schenney@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/1958903005/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1958903005/1
This fixes the reported issue. I'm looking into why this fixes the problem, as it's probably a bug.
Have you been able to trace back into the extensions code which makes the widget? Where does this widget come from?
Also: does this result in more geometry code running when loading the blogspot example in the other bug? That's I the most important.
On 2016/05/09 16:30:43, chrishtr wrote: > Also: does this result in more geometry code running when loading the blogspot > example in the > other bug? That's I the most important. I've been trying to track it back but debugging this is apparently very non-trivial. Too man renderer processes to keep track of. My guess is that it does result in more code running, which is why I'm still looking into the underlying problem. I'll focus on understanding that first.
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_...)
On 2016/05/09 16:30:03, chrishtr wrote: > Have you been able to trace back into the extensions code which makes the > widget? Where does > this widget come from? This is weird. The bug is because PoupMenuImpl::writeDocument computes the incorrect "anchorRectInScreen" rectangle, but the call in which that happens comes after the final WebPluginContainerImpl::calculateGeometry call, regardless of whether we call calculateGeometry from setFrameRect or not. So something about either calling calculateGeometry twice, or doing it once then layout then again, is causing the change in behavior in PopupMenuImpl. I will dig deeper.
On 2016/05/09 18:34:16, Stephen Chennney wrote: > On 2016/05/09 16:30:03, chrishtr wrote: > > Have you been able to trace back into the extensions code which makes the > > widget? Where does > > this widget come from? > > This is weird. The bug is because PoupMenuImpl::writeDocument computes the > incorrect "anchorRectInScreen" rectangle, but the call in which that happens > comes after the final WebPluginContainerImpl::calculateGeometry call, regardless > of whether we call calculateGeometry from setFrameRect or not. So something > about either calling calculateGeometry twice, or doing it once then layout then > again, is causing the change in behavior in PopupMenuImpl. I will dig deeper. WTF! https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/b... Why on earth do we update the delegate_ if the rectangle has changed, but send the browser a message if the rectangle is unchanged? That has to be a bug, right? It explicitly requires updateGeometry to be called twice with the same rectangle, which seems insane. Knowing my luck, if I change it the internet will break.
The CQ bit was checked by schenney@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/1958903005/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1958903005/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_...)
On 2016/05/09 at 20:10:58, schenney wrote: > On 2016/05/09 18:34:16, Stephen Chennney wrote: > > On 2016/05/09 16:30:03, chrishtr wrote: > > > Have you been able to trace back into the extensions code which makes the > > > widget? Where does > > > this widget come from? > > > > This is weird. The bug is because PoupMenuImpl::writeDocument computes the > > incorrect "anchorRectInScreen" rectangle, but the call in which that happens > > comes after the final WebPluginContainerImpl::calculateGeometry call, regardless > > of whether we call calculateGeometry from setFrameRect or not. So something > > about either calling calculateGeometry twice, or doing it once then layout then > > again, is causing the change in behavior in PopupMenuImpl. I will dig deeper. > > WTF! https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/b... > > Why on earth do we update the delegate_ if the rectangle has changed, but send the browser a message if the rectangle is unchanged? > > That has to be a bug, right? It explicitly requires updateGeometry to be called twice with the same rectangle, which seems insane. Knowing my luck, if I change it the internet will break. tommycli might have some insight, he has touched a lot of this code.
On 2016/05/09 22:21:47, chrishtr wrote: > On 2016/05/09 at 20:10:58, schenney wrote: > > On 2016/05/09 18:34:16, Stephen Chennney wrote: > > > On 2016/05/09 16:30:03, chrishtr wrote: > > > > Have you been able to trace back into the extensions code which makes the > > > > widget? Where does > > > > this widget come from? > > > > > > This is weird. The bug is because PoupMenuImpl::writeDocument computes the > > > incorrect "anchorRectInScreen" rectangle, but the call in which that happens > > > comes after the final WebPluginContainerImpl::calculateGeometry call, > regardless > > > of whether we call calculateGeometry from setFrameRect or not. So something > > > about either calling calculateGeometry twice, or doing it once then layout > then > > > again, is causing the change in behavior in PopupMenuImpl. I will dig > deeper. > > > > WTF! > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/b... > > > > Why on earth do we update the delegate_ if the rectangle has changed, but send > the browser a message if the rectangle is unchanged? > > > > That has to be a bug, right? It explicitly requires updateGeometry to be > called twice with the same rectangle, which seems insane. Knowing my luck, if I > change it the internet will break. > > tommycli might have some insight, he has touched a lot of this code. The change was made 3 or more years ago when a new IPC message was added. The text equal condition was an early return condition I.e. once did nothing. The message was added before the return for some reason, presumably because there used to be other parts to the condition. I know it's confusing - tomorrow I'll dig up the change again. Then the code was refactored until only the text compare and the message remained. I think it only ever worked because of the extra calculateGeometry call.
On 2016/05/09 23:36:58, Stephen Chennney wrote: > On 2016/05/09 22:21:47, chrishtr wrote: > > On 2016/05/09 at 20:10:58, schenney wrote: > > > On 2016/05/09 18:34:16, Stephen Chennney wrote: > > > > On 2016/05/09 16:30:03, chrishtr wrote: > > > > > Have you been able to trace back into the extensions code which makes > the > > > > > widget? Where does > > > > > this widget come from? > > > > > > > > This is weird. The bug is because PoupMenuImpl::writeDocument computes the > > > > incorrect "anchorRectInScreen" rectangle, but the call in which that > happens > > > > comes after the final WebPluginContainerImpl::calculateGeometry call, > > regardless > > > > of whether we call calculateGeometry from setFrameRect or not. So > something > > > > about either calling calculateGeometry twice, or doing it once then layout > > then > > > > again, is causing the change in behavior in PopupMenuImpl. I will dig > > deeper. > > > > > > WTF! > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/b... > > > > > > Why on earth do we update the delegate_ if the rectangle has changed, but > send > > the browser a message if the rectangle is unchanged? > > > > > > That has to be a bug, right? It explicitly requires updateGeometry to be > > called twice with the same rectangle, which seems insane. Knowing my luck, if > I > > change it the internet will break. > > > > tommycli might have some insight, he has touched a lot of this code. > > The change was made 3 or more years ago when a new IPC message was added. The > text equal condition was an early return condition I.e. once did nothing. The > message was added before the return for some reason, presumably because there > used to be other parts to the condition. I know it's confusing - tomorrow I'll > dig up the change again. > > Then the code was refactored until only the text compare and the message > remained. I think it only ever worked because of the extra calculateGeometry > call. The change that put the message inside the "rects are equal" is https://chromium.googlesource.com/chromium/src/+/32deec6509deb789265c50ba2943... Look for UpdateGeometry in that diff. I think my change is wrong though. Presumably if we are newly-attached we need to send the message even if the rects are the same. I'll have to track whether we early outed due to non-attachment.
On 2016/05/10 13:26:42, Stephen Chennney wrote: > On 2016/05/09 23:36:58, Stephen Chennney wrote: > > On 2016/05/09 22:21:47, chrishtr wrote: > > > On 2016/05/09 at 20:10:58, schenney wrote: > > > > On 2016/05/09 18:34:16, Stephen Chennney wrote: > > > > > On 2016/05/09 16:30:03, chrishtr wrote: > > > > > > Have you been able to trace back into the extensions code which makes > > the > > > > > > widget? Where does > > > > > > this widget come from? > > > > > > > > > > This is weird. The bug is because PoupMenuImpl::writeDocument computes > the > > > > > incorrect "anchorRectInScreen" rectangle, but the call in which that > > happens > > > > > comes after the final WebPluginContainerImpl::calculateGeometry call, > > > regardless > > > > > of whether we call calculateGeometry from setFrameRect or not. So > > something > > > > > about either calling calculateGeometry twice, or doing it once then > layout > > > then > > > > > again, is causing the change in behavior in PopupMenuImpl. I will dig > > > deeper. > > > > > > > > WTF! > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/b... > > > > > > > > Why on earth do we update the delegate_ if the rectangle has changed, but > > send > > > the browser a message if the rectangle is unchanged? > > > > > > > > That has to be a bug, right? It explicitly requires updateGeometry to be > > > called twice with the same rectangle, which seems insane. Knowing my luck, > if > > I > > > change it the internet will break. > > > > > > tommycli might have some insight, he has touched a lot of this code. > > > > The change was made 3 or more years ago when a new IPC message was added. The > > text equal condition was an early return condition I.e. once did nothing. The > > message was added before the return for some reason, presumably because there > > used to be other parts to the condition. I know it's confusing - tomorrow I'll > > dig up the change again. > > > > Then the code was refactored until only the text compare and the message > > remained. I think it only ever worked because of the extra calculateGeometry > > call. > > The change that put the message inside the "rects are equal" is > https://chromium.googlesource.com/chromium/src/+/32deec6509deb789265c50ba2943... > > Look for UpdateGeometry in that diff. > > I think my change is wrong though. Presumably if we are newly-attached we need > to send the message even if the rects are the same. I'll have to track whether > we early outed due to non-attachment. When the plugin is attached the view_rect is sent, so the fix as is works fine. I'm confident this change is right given all I know now.
Description was changed from ========== Fix positioning of widgets This is a partial revert (1 line) of https://codereview.chromium.org/1513573013 The failure to reportGeometry in setFrameRect resulted in bad geometry information for popups from a Chrome extension. Landing this now to fix the problem. A follow on patch is planned once we find out why this breaks (i.e. how setFrameRect is invoked without a matching frameRectsChanged call). R=chrishtr@chromium.org BUG=555201,596494 ========== to ========== Fix positioning of widgets The BrowserPlugin updateGeometry code was only sending a UpdateGeometry message to its host if the rect was the _same_ as the previous rect. This required two calls to the method in order to get the message sent, and no doubt resulted in the message being sent every time the geometry was the same and never when it was different. R=chrishtr@chromium.org BUG=555201,596494 ==========
You'll need someone else to review this version of the CL.
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by schenney@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/1958903005/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1958903005/60001
Description was changed from ========== Fix positioning of widgets The BrowserPlugin updateGeometry code was only sending a UpdateGeometry message to its host if the rect was the _same_ as the previous rect. This required two calls to the method in order to get the message sent, and no doubt resulted in the message being sent every time the geometry was the same and never when it was different. R=chrishtr@chromium.org BUG=555201,596494 ========== to ========== Fix positioning of widgets The BrowserPlugin updateGeometry code was only sending a UpdateGeometry message to its host if the rect was the _same_ as the previous rect. This required two calls to the method in order to get the message sent, and no doubt resulted in the message being sent every time the geometry was the same and never when it was different. R=fsamuel@chromium.org, lazyboy@chromium.org BUG=555201,596494 ==========
schenney@chromium.org changed reviewers: + fsamuel@chromium.org, lazyboy@chromium.org - chrishtr@chromium.org
Not my usual area of the code, but a perf improvement in Blink revealed this problem. https://codereview.chromium.org/1958903005/diff/60001/chrome/browser/apps/gue... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): https://codereview.chromium.org/1958903005/diff/60001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:395: // div.style.paddingTop = 60px + (input box height = 26px). The paddingTop in the test file is actually 60px, not 50. https://codereview.chromium.org/1958903005/diff/60001/content/renderer/browse... File content/renderer/browser_plugin/browser_plugin.cc (left): https://codereview.chromium.org/1958903005/diff/60001/content/renderer/browse... content/renderer/browser_plugin/browser_plugin.cc:385: if (old_view_rect.size() == view_rect_.size()) { This very odd condition is a remnant of https://chromium.googlesource.com/chromium/src/+/32deec6509deb789265c50ba2943..., much moved since then. It was revealed by a recent change to prevent double calls to updateGeometry, which are wasteful.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ran the flaky linux test 50 times locally with no failure, so any flakiness would seem to be trybot specific.
Thanks, one comment/question. https://codereview.chromium.org/1958903005/diff/60001/chrome/browser/apps/gue... File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): https://codereview.chromium.org/1958903005/diff/60001/chrome/browser/apps/gue... chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:395: // div.style.paddingTop = 60px + (input box height = 26px). On 2016/05/10 18:40:22, Stephen Chennney wrote: > The paddingTop in the test file is actually 60px, not 50. wow! thanks! https://codereview.chromium.org/1958903005/diff/60001/content/renderer/browse... File content/renderer/browser_plugin/browser_plugin.cc (left): https://codereview.chromium.org/1958903005/diff/60001/content/renderer/browse... content/renderer/browser_plugin/browser_plugin.cc:385: if (old_view_rect.size() == view_rect_.size()) { On 2016/05/10 18:40:23, Stephen Chennney wrote: > This very odd condition is a remnant of > https://chromium.googlesource.com/chromium/src/+/32deec6509deb789265c50ba2943..., > much moved since then. It was revealed by a recent change to prevent double > calls to updateGeometry, Which change was that? which are wasteful. The intention here IIRC was when plugin was not resized but its position (x,y) changed, we wanted to send the rect to browser/ process in this case. What you have still accomplishes the same and but a bit more. In case where the size have changed (but x, y didn't), we would send an IPC through GuestViewContainer in line 380 above: https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser... and then an IPC here, which wasn't necessary. Maybe instead you need if (old_view_rect != view_rect_ && old.size() == view_rect_.size()) Would that accomplish our goal here?
On 2016/05/10 20:51:59, lazyboy wrote: > Thanks, one comment/question. > > https://codereview.chromium.org/1958903005/diff/60001/chrome/browser/apps/gue... > File chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc (right): > > https://codereview.chromium.org/1958903005/diff/60001/chrome/browser/apps/gue... > chrome/browser/apps/guest_view/web_view_interactive_browsertest.cc:395: // > div.style.paddingTop = 60px + (input box height = 26px). > On 2016/05/10 18:40:22, Stephen Chennney wrote: > > The paddingTop in the test file is actually 60px, not 50. > > wow! thanks! > > https://codereview.chromium.org/1958903005/diff/60001/content/renderer/browse... > File content/renderer/browser_plugin/browser_plugin.cc (left): > > https://codereview.chromium.org/1958903005/diff/60001/content/renderer/browse... > content/renderer/browser_plugin/browser_plugin.cc:385: if (old_view_rect.size() > == view_rect_.size()) { > On 2016/05/10 18:40:23, Stephen Chennney wrote: > > This very odd condition is a remnant of > > > https://chromium.googlesource.com/chromium/src/+/32deec6509deb789265c50ba2943..., > > much moved since then. It was revealed by a recent change to prevent double > > calls to updateGeometry, > Which change was that? > which are wasteful. The change that reduced calls to updateGeometry is https://codereview.chromium.org/1513573013. We were calling it in WebPluginContainerImpl both when the frame size was set and again when we got the frameSizeChanged callback. > > The intention here IIRC was when plugin was not resized but its position (x,y) > changed, we wanted to send the rect to browser/ process in this case. > What you have still accomplishes the same and but a bit more. > In case where the size have changed (but x, y didn't), we would send an IPC > through GuestViewContainer in line 380 above: > https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser... > and then an IPC here, which wasn't necessary. > > Maybe instead you need > if (old_view_rect != view_rect_ && old.size() == view_rect_.size()) > Would that accomplish our goal here? Not quite. We also need to check that the delegate existed and received the size update. Patch updated to reflect this.
The CQ bit was checked by schenney@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/1958903005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1958903005/80001
lgtm
On 2016/05/19 16:03:23, lazyboy wrote: > lgtm Thanks. I still need to look into that Win test failure.
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_...)
The CQ bit was checked by schenney@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1958903005/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1958903005/80001
On 2016/05/19 16:07:13, Stephen Chennney wrote: > On 2016/05/19 16:03:23, lazyboy wrote: > > lgtm > > Thanks. I still need to look into that Win test failure. It seems that WebViewContextMenuInteractiveTest.ContextMenuParamsAfterCSSTransforms became quite flaky :( there were two failures in linux_chromium_rel_ng, it passed on third try: https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium...
Message was sent while issue was closed.
Description was changed from ========== Fix positioning of widgets The BrowserPlugin updateGeometry code was only sending a UpdateGeometry message to its host if the rect was the _same_ as the previous rect. This required two calls to the method in order to get the message sent, and no doubt resulted in the message being sent every time the geometry was the same and never when it was different. R=fsamuel@chromium.org, lazyboy@chromium.org BUG=555201,596494 ========== to ========== Fix positioning of widgets The BrowserPlugin updateGeometry code was only sending a UpdateGeometry message to its host if the rect was the _same_ as the previous rect. This required two calls to the method in order to get the message sent, and no doubt resulted in the message being sent every time the geometry was the same and never when it was different. R=fsamuel@chromium.org, lazyboy@chromium.org BUG=555201,596494 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Fix positioning of widgets The BrowserPlugin updateGeometry code was only sending a UpdateGeometry message to its host if the rect was the _same_ as the previous rect. This required two calls to the method in order to get the message sent, and no doubt resulted in the message being sent every time the geometry was the same and never when it was different. R=fsamuel@chromium.org, lazyboy@chromium.org BUG=555201,596494 ========== to ========== Fix positioning of widgets The BrowserPlugin updateGeometry code was only sending a UpdateGeometry message to its host if the rect was the _same_ as the previous rect. This required two calls to the method in order to get the message sent, and no doubt resulted in the message being sent every time the geometry was the same and never when it was different. R=fsamuel@chromium.org, lazyboy@chromium.org BUG=555201,596494 Committed: https://crrev.com/b999309cd9ace1b703df73ed50d1c0d9efde90fa Cr-Commit-Position: refs/heads/master@{#394808} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/b999309cd9ace1b703df73ed50d1c0d9efde90fa Cr-Commit-Position: refs/heads/master@{#394808}
Message was sent while issue was closed.
On 2016/05/19 17:30:52, lazyboy wrote: > On 2016/05/19 16:07:13, Stephen Chennney wrote: > > On 2016/05/19 16:03:23, lazyboy wrote: > > > lgtm > > > > Thanks. I still need to look into that Win test failure. > > It seems that > WebViewContextMenuInteractiveTest.ContextMenuParamsAfterCSSTransforms became > quite flaky :( > there were two failures in linux_chromium_rel_ng, it passed on third try: > https://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium... Locally it's not flaky. There's clearly some race condition here, but I don't understand things well enough to explain it definitively. I'll try and repro on Windows, but that won't go fast given I haven't touched Windows for months. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
