Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7068029:
Fix residue left over from find bar when using accelerated compositing (Closed)
|
Created:
9 years, 7 months ago by jbates Modified:
9 years, 6 months ago CC:
chromium-reviews, darin-cc_chromium.org, brettw-cc_chromium.org, Vangelis Kokkevis Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionThis bug fixes a couple issues when accelerated compositing is enabled:
- residue left over from find bar.
- no redraw when another window is dragged over chrome tab.
BUG=83091
TEST=see bug for manual test
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=87084
Patch Set 1 #
Total comments: 13
Patch Set 2 : comments #
Total comments: 4
Patch Set 3 : . #
Total comments: 3
Patch Set 4 : simplified #Patch Set 5 : cleanup #
Total comments: 9
Patch Set 6 : . #
Total comments: 1
Patch Set 7 : . #
Messages
Total messages: 22 (0 generated)
(added Al as a reviewer)
LGTM less me bikeshedding. http://codereview.chromium.org/7068029/diff/1/chrome/browser/renderer_host/re... File chrome/browser/renderer_host/render_widget_host_view_win.cc (right): http://codereview.chromium.org/7068029/diff/1/chrome/browser/renderer_host/re... chrome/browser/renderer_host/render_widget_host_view_win.cc:73: const char* kRenderWidgetHVWPropStr = "RWHVW_P"; Chrome_RenderWidgetHostViewCompositorHWND_Parent use const wchar_t and L prefix? http://codereview.chromium.org/7068029/diff/1/chrome/browser/renderer_host/re... chrome/browser/renderer_host/render_widget_host_view_win.cc:1500: RemovePropA(hWnd, kRenderWidgetHVWPropStr); s/PropA/Prop if you use wchar as above http://codereview.chromium.org/7068029/diff/1/chrome/browser/renderer_host/re... chrome/browser/renderer_host/render_widget_host_view_win.cc:1510: void RenderWidgetHostViewWin::ScheduleComposite() { ScheduleCompositeIfNotAlreadyDone? A few words of what this code does might be good. http://codereview.chromium.org/7068029/diff/1/chrome/browser/renderer_host/re... File chrome/browser/renderer_host/render_widget_host_view_win.h (right): http://codereview.chromium.org/7068029/diff/1/chrome/browser/renderer_host/re... chrome/browser/renderer_host/render_widget_host_view_win.h:275: void ResetWasCompositingJustCompleted(); s/completed/scheduled/ http://codereview.chromium.org/7068029/diff/1/chrome/browser/renderer_host/re... chrome/browser/renderer_host/render_widget_host_view_win.h:286: // If a composite operation was just done. You migth want to explain what this means in detail given how hinky it is.
http://codereview.chromium.org/7068029/diff/1/chrome/browser/renderer_host/re... File chrome/browser/renderer_host/render_widget_host_view_win.cc (right): http://codereview.chromium.org/7068029/diff/1/chrome/browser/renderer_host/re... chrome/browser/renderer_host/render_widget_host_view_win.cc:1510: void RenderWidgetHostViewWin::ScheduleComposite() { On 2011/05/25 23:42:29, nduca wrote: > ScheduleCompositeIfNotAlreadyDone? > > A few words of what this code does might be good. I agree that a comment here would be needed. I cannot tell what this code does or how it fixes the issue. http://codereview.chromium.org/7068029/diff/1/chrome/browser/renderer_host/re... File chrome/browser/renderer_host/render_widget_host_view_win.h (right): http://codereview.chromium.org/7068029/diff/1/chrome/browser/renderer_host/re... chrome/browser/renderer_host/render_widget_host_view_win.h:78: // Schedule a composite no the compositor hwnd unless one was just done. Is there a typo in this comment? I'm not sure what it's supposed to mean...
These redraw bugs are from a regression introduced on April 27. Probably related to http://codereview.chromium.org/6880218/, but bisecting builds seems to indicate between r83227 and r83250. What exactly is causing this bug? Windows is giving our child (compositor) HWND WM_PAINT messages while the find window animates off screen and while any other window is dragged over Chrome. These events were being received by CompositorHostWindowProc, but were not triggering an actual compositor redraw. How to fix? This change simply triggers a composite when we get a WM_PAINT message on the compositor window. Why did it work before Apr 27? Maybe it worked before Apr 27 because we had an extra HWND involved and somehow it was triggering OnPaint on the parent window -- but I'll leave the full explanation as an exercise for one of our expert Windows programmers. http://codereview.chromium.org/7068029/diff/1/chrome/browser/renderer_host/re... File chrome/browser/renderer_host/render_widget_host_view_win.cc (right): http://codereview.chromium.org/7068029/diff/1/chrome/browser/renderer_host/re... chrome/browser/renderer_host/render_widget_host_view_win.cc:73: const char* kRenderWidgetHVWPropStr = "RWHVW_P"; On 2011/05/25 23:42:29, nduca wrote: > Chrome_RenderWidgetHostViewCompositorHWND_Parent > > use const wchar_t and L prefix? This string isn't actually used anywhere - only the pointer is used by windows to be unique among other properties of the compositor window. I didn't want to waste extra bytes of memory to make a descriptive string that is not used. http://codereview.chromium.org/7068029/diff/1/chrome/browser/renderer_host/re... chrome/browser/renderer_host/render_widget_host_view_win.cc:1500: RemovePropA(hWnd, kRenderWidgetHVWPropStr); On 2011/05/25 23:42:29, nduca wrote: > s/PropA/Prop if you use wchar as above The MSDN example for SetProp uses char* strings and it turns out that it takes char* or wchar* depending on compile flags. Rather than get into all that, I decided to just be explicit with SetPropA since unicode precision is not an issue. Incidentally, you can specify _T("bla bla") to let the compiler decide (and that matches the LPCTSTR type of SetProp). But I think that's a bit overkill for our needs here. http://codereview.chromium.org/7068029/diff/1/chrome/browser/renderer_host/re... chrome/browser/renderer_host/render_widget_host_view_win.cc:1510: void RenderWidgetHostViewWin::ScheduleComposite() { On 2011/05/25 23:42:29, nduca wrote: > ScheduleCompositeIfNotAlreadyDone? > > A few words of what this code does might be good. Done. http://codereview.chromium.org/7068029/diff/1/chrome/browser/renderer_host/re... File chrome/browser/renderer_host/render_widget_host_view_win.h (right): http://codereview.chromium.org/7068029/diff/1/chrome/browser/renderer_host/re... chrome/browser/renderer_host/render_widget_host_view_win.h:78: // Schedule a composite no the compositor hwnd unless one was just done. On 2011/05/26 05:57:07, vangelis wrote: > Is there a typo in this comment? I'm not sure what it's supposed to mean... Done. http://codereview.chromium.org/7068029/diff/1/chrome/browser/renderer_host/re... chrome/browser/renderer_host/render_widget_host_view_win.h:275: void ResetWasCompositingJustCompleted(); On 2011/05/25 23:42:29, nduca wrote: > s/completed/scheduled/ Done. http://codereview.chromium.org/7068029/diff/1/chrome/browser/renderer_host/re... chrome/browser/renderer_host/render_widget_host_view_win.h:286: // If a composite operation was just done. On 2011/05/25 23:42:29, nduca wrote: > You migth want to explain what this means in detail given how hinky it is. Done.
http://codereview.chromium.org/7068029/diff/5001/chrome/browser/renderer_host... File chrome/browser/renderer_host/render_widget_host_view_win.cc (right): http://codereview.chromium.org/7068029/diff/5001/chrome/browser/renderer_host... chrome/browser/renderer_host/render_widget_host_view_win.cc:73: const char* kRenderWidgetHVWPropStr = "RWHVW_P"; A quick look through the code indicates that we don't actually specifically use the *W() or *A() variants of win32 calls. I think it's best to use the _T() macro. http://codereview.chromium.org/7068029/diff/5001/chrome/browser/renderer_host... chrome/browser/renderer_host/render_widget_host_view_win.cc:1522: void RenderWidgetHostViewWin::ResetWasCompositingJustCompleted() { Should be renamed to *JustScheduled for consistency
http://codereview.chromium.org/7068029/diff/5001/chrome/browser/renderer_host... File chrome/browser/renderer_host/render_widget_host_view_win.cc (right): http://codereview.chromium.org/7068029/diff/5001/chrome/browser/renderer_host... chrome/browser/renderer_host/render_widget_host_view_win.cc:73: const char* kRenderWidgetHVWPropStr = "RWHVW_P"; On 2011/05/26 19:21:53, vangelis wrote: > A quick look through the code indicates that we don't actually specifically use > the *W() or *A() variants of win32 calls. I think it's best to use the _T() > macro. Done. http://codereview.chromium.org/7068029/diff/5001/chrome/browser/renderer_host... chrome/browser/renderer_host/render_widget_host_view_win.cc:1522: void RenderWidgetHostViewWin::ResetWasCompositingJustCompleted() { On 2011/05/26 19:21:53, vangelis wrote: > Should be renamed to *JustScheduled for consistency Done.
http://codereview.chromium.org/7068029/diff/10001/chrome/browser/renderer_hos... File chrome/browser/renderer_host/render_widget_host_view_win.cc (right): http://codereview.chromium.org/7068029/diff/10001/chrome/browser/renderer_hos... chrome/browser/renderer_host/render_widget_host_view_win.cc:1516: MessageLoop::current()->PostTask(FROM_HERE, Maybe I don't understand what you are trying to accomplish, but won't ResetWasCompositingJustScheduled be racing with receipt of the ViewHostMsg_UpdateRect IPC? RenderWidgetHost::ScheduleComposite will sometimes process ViewHostMsg_UpdateRect before returning, but that is not guaranteed. Maybe it would be helpful if I understood why you need to limit calls to ScheduleComposite. http://codereview.chromium.org/7068029/diff/10001/chrome/browser/renderer_hos... File chrome/browser/renderer_host/render_widget_host_view_win.h (right): http://codereview.chromium.org/7068029/diff/10001/chrome/browser/renderer_hos... chrome/browser/renderer_host/render_widget_host_view_win.h:291: // This bool is set to true on the first call to ScheduleComposite, and an ScheduleComposite -> ScheduleCompositeIfNeeded, right?
After talking with Vangelis, we realized that this code could be much simpler. The latest CL is not only simpler, but it seems to reduce the tab-switching-refresh bug (http://code.google.com/p/chromium/issues/detail?id=83248) to the point that I can't repro it anymore. There's probably still a race condition though, so the other bug will remain open. http://codereview.chromium.org/7068029/diff/10001/chrome/browser/renderer_hos... File chrome/browser/renderer_host/render_widget_host_view_win.cc (right): http://codereview.chromium.org/7068029/diff/10001/chrome/browser/renderer_hos... chrome/browser/renderer_host/render_widget_host_view_win.cc:1516: MessageLoop::current()->PostTask(FROM_HERE, On 2011/05/26 23:03:06, darin wrote: > Maybe I don't understand what you are trying to accomplish, but won't > ResetWasCompositingJustScheduled be racing with receipt of the > ViewHostMsg_UpdateRect IPC? > > RenderWidgetHost::ScheduleComposite will sometimes process > ViewHostMsg_UpdateRect before returning, but that is not guaranteed. > > Maybe it would be helpful if I understood why you need to limit calls to > ScheduleComposite. You're right - it's a mistake. The latest version is much simpler. http://codereview.chromium.org/7068029/diff/13001/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host.cc (left): http://codereview.chromium.org/7068029/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host.cc:454: OnMessageReceived(msg); This blocking wait did not seem to have any effect in Linux. With or without, dragging a window over chrome resulted in some minor white trails. Since it adds more complexity (and has a race condition by design), it seems best to never block until we can show a clear advantage.
Looks gooder to me... LGTM.
Looks good to me too. Thanks!
OK, LGTM http://codereview.chromium.org/7068029/diff/13001/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host.cc (left): http://codereview.chromium.org/7068029/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host.cc:454: OnMessageReceived(msg); won't this exacerbate the issue where the tab strip is out-of-sync with the tab contents? you switch tabs, but it still shows the old tab contents until compositing eventually happens. maybe we can add back some kind of blocking like this to help synchronize the tab strip with the tab contents once we have a better idea of what we need?
http://codereview.chromium.org/7068029/diff/13001/chrome/browser/renderer_hos... File chrome/browser/renderer_host/render_widget_host_view_win.cc (right): http://codereview.chromium.org/7068029/diff/13001/chrome/browser/renderer_hos... chrome/browser/renderer_host/render_widget_host_view_win.cc:1563: SetProp(compositor_host_window_, kRenderWidgetHVWPropStr, this); SetProp() can be problematic, and can lead to strange crashes. There is a limit to the amount of memory (system-wide I think!) that can be used for property storage and so this function will eventually fail. We have a class called ViewProp in src/ui that lets you associate properties with arbitrary gfx::NativeViews, but in this case why not just set this as the window's user data? We typically store a pointer to the C++ class that subclasses WindowImpl as the user data.
http://codereview.chromium.org/7068029/diff/13001/chrome/browser/renderer_hos... File chrome/browser/renderer_host/render_widget_host_view_win.cc (right): http://codereview.chromium.org/7068029/diff/13001/chrome/browser/renderer_hos... chrome/browser/renderer_host/render_widget_host_view_win.cc:1563: SetProp(compositor_host_window_, kRenderWidgetHVWPropStr, this); On 2011/05/27 17:56:18, Ben Goodger wrote: > SetProp() can be problematic, and can lead to strange crashes. There is a limit > to the amount of memory (system-wide I think!) that can be used for property > storage and so this function will eventually fail. We have a class called > ViewProp in src/ui that lets you associate properties with arbitrary > gfx::NativeViews, but in this case why not just set this as the window's user > data? We typically store a pointer to the C++ class that subclasses WindowImpl > as the user data. User data sounds good to me. You mean SetWindowLongPtr(hwnd, GWLP_USERDATA,...) right? http://codereview.chromium.org/7068029/diff/13001/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host.cc (left): http://codereview.chromium.org/7068029/diff/13001/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host.cc:454: OnMessageReceived(msg); On 2011/05/27 17:53:08, darin wrote: > won't this exacerbate the issue where the tab strip is out-of-sync with the tab > contents? you switch tabs, but it still shows the old tab contents until > compositing eventually happens. maybe we can add back some kind of blocking > like this to help synchronize the tab strip with the tab contents once we have a > better idea of what we need? I think this code only subtly changes the behavior of switching tabs. With blocking, the UI is blocked while waiting for the new tab to render for up to 40ms. If the new tab renders within 40ms, then the UI will update at about the same time as the new tab view. If the tab takes too long, the UI will update first, and then the tab view when it's finally rendered. In both cases, the new tab only shows up when it's actually rendered. Since 40ms is not that long, I think most people won't notice the difference when switching tabs. For those that do notice the difference, they might prefer that the UI always responds fast regardless of the content.
http://codereview.chromium.org/7068029/diff/13001/chrome/browser/renderer_hos... File chrome/browser/renderer_host/render_widget_host_view_win.cc (right): http://codereview.chromium.org/7068029/diff/13001/chrome/browser/renderer_hos... chrome/browser/renderer_host/render_widget_host_view_win.cc:1563: SetProp(compositor_host_window_, kRenderWidgetHVWPropStr, this); Yes. There should be a win_util in base somewhere that does this for you.
http://codereview.chromium.org/7068029/diff/13001/chrome/browser/renderer_hos... File chrome/browser/renderer_host/render_widget_host_view_win.cc (right): http://codereview.chromium.org/7068029/diff/13001/chrome/browser/renderer_hos... chrome/browser/renderer_host/render_widget_host_view_win.cc:1563: SetProp(compositor_host_window_, kRenderWidgetHVWPropStr, this); On 2011/05/27 18:20:15, Ben Goodger wrote: > Yes. There should be a win_util in base somewhere that does this for you. I don't see anything like that in win_util or base. Is it OK to call SetWindowLongPtr directly?
On Fri, May 27, 2011 at 11:18 AM, <jbates@chromium.org> wrote: > > > http://codereview.chromium.org/7068029/diff/13001/chrome/browser/renderer_hos... > File chrome/browser/renderer_host/render_widget_host_view_win.cc > (right): > > > http://codereview.chromium.org/7068029/diff/13001/chrome/browser/renderer_hos... > chrome/browser/renderer_host/render_widget_host_view_win.cc:1563: > SetProp(compositor_host_window_, kRenderWidgetHVWPropStr, this); > On 2011/05/27 17:56:18, Ben Goodger wrote: > >> SetProp() can be problematic, and can lead to strange crashes. There >> > is a limit > >> to the amount of memory (system-wide I think!) that can be used for >> > property > >> storage and so this function will eventually fail. We have a class >> > called > >> ViewProp in src/ui that lets you associate properties with arbitrary >> gfx::NativeViews, but in this case why not just set this as the >> > window's user > >> data? We typically store a pointer to the C++ class that subclasses >> > WindowImpl > >> as the user data. >> > > User data sounds good to me. You mean SetWindowLongPtr(hwnd, > GWLP_USERDATA,...) right? > > > http://codereview.chromium.org/7068029/diff/13001/content/browser/renderer_ho... > File content/browser/renderer_host/render_widget_host.cc (left): > > > http://codereview.chromium.org/7068029/diff/13001/content/browser/renderer_ho... > content/browser/renderer_host/render_widget_host.cc:454: > OnMessageReceived(msg); > > On 2011/05/27 17:53:08, darin wrote: > >> won't this exacerbate the issue where the tab strip is out-of-sync >> > with the tab > >> contents? you switch tabs, but it still shows the old tab contents >> > until > >> compositing eventually happens. maybe we can add back some kind of >> > blocking > >> like this to help synchronize the tab strip with the tab contents once >> > we have a > >> better idea of what we need? >> > > I think this code only subtly changes the behavior of switching tabs. > With blocking, the UI is blocked while waiting for the new tab to render > for up to 40ms. If the new tab renders within 40ms, then the UI will > update at about the same time as the new tab view. If the tab takes too > long, the UI will update first, and then the tab view when it's finally > rendered. In both cases, the new tab only shows up when it's actually > rendered. Since 40ms is not that long, I think most people won't notice > the difference when switching tabs. For those that do notice the > difference, they might prefer that the UI always responds fast > regardless of the content. The software renderer will paint white when switching tabs if the renderer is slow to paint. The accelerated renderer will not paint white synchronously. So, the user will see the old contents with the new tab. That is bad. We need to fix that somehow.... obviously, not in this CL :-) -Darin > > > http://codereview.chromium.org/7068029/ >
http://codereview.chromium.org/7068029/diff/13001/chrome/browser/renderer_hos... File chrome/browser/renderer_host/render_widget_host_view_win.cc (right): http://codereview.chromium.org/7068029/diff/13001/chrome/browser/renderer_hos... chrome/browser/renderer_host/render_widget_host_view_win.cc:1563: SetProp(compositor_host_window_, kRenderWidgetHVWPropStr, this); Ah it got migrated to src/ui. But you're in src/chrome, so you're allowed to use that: src/ui/base/win/hwnd_util.h: void* ui::SetWindowUserData(HWND hwnd, void* data); void* ui::GetWindowUserData(HWND hwnd); ui::GetWindowUserData does some checking to make sure if another process clobbers your user data, you don't try a cast that'll crash.
http://codereview.chromium.org/7068029/diff/13001/chrome/browser/renderer_hos... File chrome/browser/renderer_host/render_widget_host_view_win.cc (right): http://codereview.chromium.org/7068029/diff/13001/chrome/browser/renderer_hos... chrome/browser/renderer_host/render_widget_host_view_win.cc:1563: SetProp(compositor_host_window_, kRenderWidgetHVWPropStr, this); On 2011/05/27 18:44:17, Ben Goodger wrote: > Ah it got migrated to src/ui. But you're in src/chrome, so you're allowed to use > that: > > src/ui/base/win/hwnd_util.h: > > void* ui::SetWindowUserData(HWND hwnd, void* data); > void* ui::GetWindowUserData(HWND hwnd); > > ui::GetWindowUserData does some checking to make sure if another process > clobbers your user data, you don't try a cast that'll crash. Done.
I don't see where the user data is cleared. Do we know that the pointer stored in user data will always outlive the HWND?
LGTM for SetWindowUserData usage. http://codereview.chromium.org/7068029/diff/20001/chrome/browser/renderer_hos... File chrome/browser/renderer_host/render_widget_host_view_win.cc (right): http://codereview.chromium.org/7068029/diff/20001/chrome/browser/renderer_hos... chrome/browser/renderer_host/render_widget_host_view_win.cc:1491: ui::GetWindowUserData(hWnd)); quick nit: hwnd instead of hWnd.
Btw you can just set to NULL if you want to remove it in addressing Darin's comment. (There is no method to remove user data). -Ben On Fri, May 27, 2011 at 12:22 PM, <ben@chromium.org> wrote: > LGTM for SetWindowUserData usage. > > > > http://codereview.chromium.org/7068029/diff/20001/chrome/browser/renderer_hos... > > File chrome/browser/renderer_host/render_widget_host_view_win.cc > (right): > > > http://codereview.chromium.org/7068029/diff/20001/chrome/browser/renderer_hos... > chrome/browser/renderer_host/render_widget_host_view_win.cc:1491: > ui::GetWindowUserData(hWnd)); > quick nit: hwnd instead of hWnd. > > > http://codereview.chromium.org/7068029/ > |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
