This adds in the ability for Chrome to generate windows with snapshots of all...

This adds in the ability for Chrome to generate windows with snapshots
of all currently open tabs in all browsers.

This is needed for overview mode on ChromeOS.

BUG=http://code.google.com/p/chromium-os/issues/detail?id=1170
TEST=Ran Chrome under ChromeOS with updated window manager.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=45824

[Greg Spencer (Chromium), 2010-03-18 01:40:38]

This change will be paired with a new version of the window manager (layout
manager, really) code on the ChromeOS side.

http://codereview.chromium.org/661237/diff/49001/50003
File chrome/browser/chromeos/wm_ipc.h (right):

http://codereview.chromium.org/661237/diff/49001/50003#newcode225
chrome/browser/chromeos/wm_ipc.h:225: //   param[0]: tab index of newly
magnified window.
Note that this message isn't handled at all by the current message listener, so
changing the meaning of the parameters is not an issue.

[Greg Spencer (Chromium), 2010-03-19 01:10:36]

Brett, hold off on reviewing this -- I found that I wasn't really doing what I
thought, so I'm fixing it.  I'll let you know when it's ready.

Sorry if you already started...

[Daniel Erat, 2010-03-19 04:49:09]

I know you said that you're reworking this, but here are a few comments that I
had from the small portion that I'd gone through, in case they're relevant.

http://codereview.chromium.org/661237/diff/45003/61003
File chrome/browser/chromeos/wm_ipc.h (right):

http://codereview.chromium.org/661237/diff/45003/61003#newcode190
chrome/browser/chromeos/wm_ipc.h:190: //   param[1]: was mode cancelled? (0 =
no, 1 = yes)
can you elaborate on what cancelled means here?

http://codereview.chromium.org/661237/diff/45003/61003#newcode249
chrome/browser/chromeos/wm_ipc.h:249: // Notify the window manager when the tabs
order may have changed.
is this associated with a particular window?

http://codereview.chromium.org/661237/diff/45003/61005
File chrome/browser/chromeos/wm_message_listener.cc (right):

http://codereview.chromium.org/661237/diff/45003/61005#newcode7
chrome/browser/chromeos/wm_message_listener.cc:7: #include
"chrome/browser/chromeos/wm_overview_controller.h"
nit: alphabetize these

http://codereview.chromium.org/661237/diff/45003/61001
File chrome/browser/chromeos/wm_message_listener.h (right):

http://codereview.chromium.org/661237/diff/45003/61001#newcode61
chrome/browser/chromeos/wm_message_listener.h:61: // Hids the overview windows.
s/Hids/Hides/

http://codereview.chromium.org/661237/diff/45003/61002
File chrome/browser/chromeos/wm_overview_controller.cc (right):

http://codereview.chromium.org/661237/diff/45003/61002#newcode53
chrome/browser/chromeos/wm_overview_controller.cc:53: Browser* GetBrowserAt(int
tab_index, int* browser_index) const;
can you rename 'browser_index' to 'tab_index_within_browser', just to make it
clear than it's not the Browser object's index?

http://codereview.chromium.org/661237/diff/45003/61002#newcode55
chrome/browser/chromeos/wm_overview_controller.cc:55: // Return true if the tab
a the given snapshot index is the selected
"tab a the given" -> "tab at the given"

http://codereview.chromium.org/661237/diff/45003/61002#newcode115
chrome/browser/chromeos/wm_overview_controller.cc:115: // This Child was
created.
s/This/this/

http://codereview.chromium.org/661237/diff/45003/61006
File chrome/browser/chromeos/wm_overview_controller.h (right):

http://codereview.chromium.org/661237/diff/45003/61006#newcode59
chrome/browser/chromeos/wm_overview_controller.h:59: void SaveCurrentTabs();
add a comment for SaveCurrentTabs()?

http://codereview.chromium.org/661237/diff/45003/61006#newcode61
chrome/browser/chromeos/wm_overview_controller.h:61: // Selects the Browser at
|tab_index|.
should this really be "Focus the browser containing the tab at |tab_index|"?

http://codereview.chromium.org/661237/diff/45003/61006#newcode74
chrome/browser/chromeos/wm_overview_controller.h:74: // Reconfigure any
zero-sized snapshots, in case they have become
which method does this comment belong to?

[Greg Spencer (Chromium), 2010-04-10 00:31:10]

OK, Brett, go ahead and start on this now.  Sorry it took so long to get ready
-- had to change a bunch of things.

(BTW, all of Daniel's comments have been invalidated by the code changes)

[brettw, 2010-04-16 17:52:29]

I'm really sorry I took so long. I was focusing on Pepper stuff this week and
this code review took a lot of thought. I will definitely be faster for followup
passes.

http://codereview.chromium.org/661237/diff/184004/179008
File chrome/browser/chromeos/wm_overview_controller.h (right):

http://codereview.chromium.org/661237/diff/184004/179008#newcode54
chrome/browser/chromeos/wm_overview_controller.h:54: // Called immediately after
a browser is added to the list
Be sure your comments end with periods.

http://codereview.chromium.org/661237/diff/184004/179008#newcode142
chrome/browser/chromeos/wm_overview_controller.h:142: }  // end namespace
chromeos
You can remove "end"

http://codereview.chromium.org/661237/diff/184004/179009
File chrome/browser/chromeos/wm_overview_snapshot.cc (right):

http://codereview.chromium.org/661237/diff/184004/179009#newcode78
chrome/browser/chromeos/wm_overview_snapshot.cc:78: }  // end namespace chromeos
Remove "end" and put a blank line before this.

http://codereview.chromium.org/661237/diff/184004/179007
File chrome/browser/chromeos/wm_overview_snapshot.h (right):

http://codereview.chromium.org/661237/diff/184004/179007#newcode46
chrome/browser/chromeos/wm_overview_snapshot.h:46: }  // end namespace chromeos
You can remove "end"

http://codereview.chromium.org/661237/diff/184004/179010
File chrome/browser/tab_contents/thumbnail_generator.cc (right):

http://codereview.chromium.org/661237/diff/184004/179010#newcode104
chrome/browser/tab_contents/thumbnail_generator.cc:104: SkBitmap
snapshot_result;
I don't think we need the distinction between a snapshot and a thumbnail. The
thumbnail generator doesn't guarantee any particular sized result, just that a
version of the page image can always be retrieved.

I would use the old code but just change the thumbnail size at the top of the
file when compiling under ChromeOS to the size we need.

At the same time, I think you can then remove all the Chrome OS defines in this
file and its header.

http://codereview.chromium.org/661237/diff/184004/179010#newcode217
chrome/browser/tab_contents/thumbnail_generator.cc:217: BackingStore*
backing_store = renderer->GetBackingStore(true);
If you look at the comments in render_widget(), I think there should be a
special message for getting the snapshot. In this case, you should send that
message and remove this set_render_if_hidden hack.

We should not be getting the backing stores here in any case with force_create.
We maintain a LRU cache of backing stores to make tab switching faster. Doing
this for a bunch of tabs will blow away that cache, which isn't what we want.
We'd rather these thumbnail requests bypass the cache, which is what the
separate thumbnail request message I suggested would do.

The last problem with this is that doing force_create here doesn't do what you
want anyway. Painting is asynchronous. When you ask for a backing store with
force create and it doesn't exist, we will send a paint request to the renderer.
This will come back at some later time. As an optimization, we have a 50ms
timeout where we wait for the renderer to try to get more paints synchronously.
I suspect in your debugging you were falling under this 50ms and the results
were coming back out. But if painting takes more than 50ms, what this code does
is paint, saves it in the backing store cache (booting out something you
probably wanted more), and then doesn't use the result for a snapshot (since
that bitmap came back after this plugin returned).

http://codereview.chromium.org/661237/diff/184004/179017
File chrome/renderer/render_widget.h (right):

http://codereview.chromium.org/661237/diff/184004/179017#newcode269
chrome/renderer/render_widget.h:269: bool do_next_paint_even_if_hidden_;
This flag seems a bit hacky. I think it would be cleaner all around if the new
message was called CaptureImage (or something similar) and the RenderWidget
implemented that independent of the current painting system of invalidations.
Instead, it would just do Layout, then paint directly into a new bitmap it makes
on the spot, and then sends that new bitmap back via a new ViewHostMsg response
that somehow gets routed to the thumbnail generator.

This new message could also take a "shrink to" minimum size, and the
RenderWidget could call DownsampleBy2Until... on that size. This gets the resize
CPU hit off of the UI thread.

[Greg Spencer (Chromium), 2010-04-17 00:47:57]

So, if it makes sense to add a new message round-trip to the renderer,
it seems to make sense to me to make thumbnail generation be async as
well, so that I don't have to block the UI thread while the render
occurs.

The way I think this should work is:

1) The overview controller gets the thumbnail generator from the
   browser process.
2) It creates a callback object for receiving the results of the
   PaintAtScale on the renderer, and calls a new method,
   AskForThumbnail, on the thumbnail generator giving it this
   callback object.  If AskForThumbnail can create the thumbnail from
   already present data (backing store, cached thumbnail), then it
   does so, and invokes the callback immediately.
3) If not, the thumbnail generator uses the RenderWidgetHost to send
   the new PaintAtScale message to the renderer, passing the callback
   to the host to be called later, and the AskForThumbnail method returns.
4) Time passes while the renderer renders and scales the bitmap...
5) When the PaintAtScaleACK message returns from the renderer, the
   RenderWidgetHost invokes the callback with the resulting SkBitmap.
6) When the callback is invoked, it sets the image on the snapshot's
   ImageView control.
7) Once the callback is invoked, the RenderWidgetHost removes it from
   its map of pending PaintAtScale messages.

To do this, I think I need:

- Add AskForThumbnail on the thumbnail generator.
- Add a PaintAtScale message to the renderer which does a layout and
  then renders into a canvas which we can take the bitmap from, scale
  it to the scale requested, and then send the PaintAtScaleACK with a
  DIB in it.
- Add a handler for a PaintAtScaleACK to the render_widget_host
- Add a map where the key is a pair:(renderer_id, scale) and the
  values are pending PaintAtScale callback to the RenderWidgetHost
  class so that when a PaintAtScaleACK is received, it can invoke the
  appropriate callback.

Alternatively to the callbacks being kept by the RenderWidgetHost (or
maybe RenderWidgetHelper), I could just add a method which sets the
"paint_at_scale_observer" on the RenderWidgetHost, and that gets
called with the bitmap when the PaintAtScaleACK comes in, and we can
register the thumbnail generator as this observer.  The thumbnail
generator would then keep the callbacks instead.

Let me know if this plan seems plausible, or if you have better suggestions.

-Greg.

[brettw, 2010-04-19 16:44:27]

On Fri, Apr 16, 2010 at 5:47 PM,  <gspencer@chromium.org> wrote:
> So, if it makes sense to add a new message round-trip to the renderer,
> it seems to make sense to me to make thumbnail generation be async as
> well, so that I don't have to block the UI thread while the render
> occurs.
>
> The way I think this should work is:
>
> 1) The overview controller gets the thumbnail generator from the
>   browser process.
> 2) It creates a callback object for receiving the results of the
>   PaintAtScale on the renderer, and calls a new method,
>   AskForThumbnail, on the thumbnail generator giving it this
>   callback object.  If AskForThumbnail can create the thumbnail from
>   already present data (backing store, cached thumbnail), then it
>   does so, and invokes the callback immediately.
> 3) If not, the thumbnail generator uses the RenderWidgetHost to send
>   the new PaintAtScale message to the renderer, passing the callback
>   to the host to be called later, and the AskForThumbnail method returns.
> 4) Time passes while the renderer renders and scales the bitmap...
> 5) When the PaintAtScaleACK message returns from the renderer, the
>   RenderWidgetHost invokes the callback with the resulting SkBitmap.
> 6) When the callback is invoked, it sets the image on the snapshot's
>   ImageView control.
> 7) Once the callback is invoked, the RenderWidgetHost removes it from
>   its map of pending PaintAtScale messages.
>
> To do this, I think I need:
>
> - Add AskForThumbnail on the thumbnail generator.
> - Add a PaintAtScale message to the renderer which does a layout and
>  then renders into a canvas which we can take the bitmap from, scale
>  it to the scale requested, and then send the PaintAtScaleACK with a
>  DIB in it.
> - Add a handler for a PaintAtScaleACK to the render_widget_host
> - Add a map where the key is a pair:(renderer_id, scale) and the
>  values are pending PaintAtScale callback to the RenderWidgetHost
>  class so that when a PaintAtScaleACK is received, it can invoke the
>  appropriate callback.
>
> Alternatively to the callbacks being kept by the RenderWidgetHost (or
> maybe RenderWidgetHelper), I could just add a method which sets the
> "paint_at_scale_observer" on the RenderWidgetHost, and that gets
> called with the bitmap when the PaintAtScaleACK comes in, and we can
> register the thumbnail generator as this observer.  The thumbnail
> generator would then keep the callbacks instead.
>
> Let me know if this plan seems plausible, or if you have better suggestions.

This sounds basically correct to me.

One of the things I'm worried about is adding too much tracking crap
in RVH or TabContents for tracking this. The best option I can come up
with is that the RVH forwards the "paintatscale_ack" message to the TC
via its delegate, and TC forwards that to the thumbnail generator.
Then the thumbnail generator tracks who's listening for which messages
from where and invokes the correct callback.

Brett

[Greg Spencer (Chromium), 2010-04-23 00:08:52]

OK, Brett, I've implemented what I outlined earlier for you,
so go ahead and review this again.

-Greg.

[brettw, 2010-04-26 17:21:57]

These are all lower-level comments, I don't have any more comments about the
high level design, which looks good.

I didn't get to the wm_* files you added yet, will do those now.

http://codereview.chromium.org/661237/diff/298001/251033
File app/surface/transport_dib.h (right):

http://codereview.chromium.org/661237/diff/298001/251033#newcode79
app/surface/transport_dib.h:79: static Handle TestHandleValue() {
At first I thought this function was "testing" a handle value (like to see if
it's null or something). How about calling it GetFakeHandleForTest or something,
which also makes more clear it's not a real handle value

http://codereview.chromium.org/661237/diff/298001/251031
File chrome/browser/renderer_host/backing_store_x.cc (right):

http://codereview.chromium.org/661237/diff/298001/251031#newcode349
chrome/browser/renderer_host/backing_store_x.cc:349: *bitmap.getAddr32(x, y) =
src_row[x] | 0xff000000;
Can you put back the thing where we only call getAddr32 at the beginning of each
row? I think there is more work involved in this computation, so normally we
only do it at the beginning of each line.

http://codereview.chromium.org/661237/diff/298001/251013
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/661237/diff/298001/251013#newcode216
chrome/browser/renderer_host/render_widget_host.h:216: void
PaintAtSize(TransportDIB::Handle dib_handle, gfx::Size size);
We usually pass Size as a const reference.

http://codereview.chromium.org/661237/diff/298001/251012
File chrome/browser/tab_contents/thumbnail_generator.cc (right):

http://codereview.chromium.org/661237/diff/298001/251012#newcode301
chrome/browser/tab_contents/thumbnail_generator.cc:301:
callback_map_.erase(item);
I think we'll want some handling for the case where a renderer crashed. It seems
like the callback should still get called or at least removed from the list. I
think TabContents::RenderViewGone is the right place to add a call to the
thumbnail generator for this.

http://codereview.chromium.org/661237/diff/298001/251029
File chrome/browser/tab_contents/thumbnail_generator.h (right):

http://codereview.chromium.org/661237/diff/298001/251029#newcode8
chrome/browser/tab_contents/thumbnail_generator.h:8: #include <vector>
These should be in order.

http://codereview.chromium.org/661237/diff/298001/251029#newcode107
chrome/browser/tab_contents/thumbnail_generator.h:107: Lock callback_map_lock_;
I'm not sure you need this lock, I don't think this object is used anywhere but
the UI thread. Is that true?

http://codereview.chromium.org/661237/diff/298001/251019
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/661237/diff/298001/251019#newcode500
chrome/renderer/render_widget.cc:500: if (pending_input_event_ack_.get()) {
You removed the CallDoDeferredUpdate function. There are a number of early
returns in this function that mean this code no longer gets called. That seems
like a problem.

[brettw, 2010-04-26 17:43:48]

http://codereview.chromium.org/661237/diff/298001/251007
File chrome/browser/chromeos/wm_overview_controller.cc (right):

http://codereview.chromium.org/661237/diff/298001/251007#newcode90
chrome/browser/chromeos/wm_overview_controller.cc:90: private:
Style: no blank line after this.

http://codereview.chromium.org/661237/diff/298001/251007#newcode364
chrome/browser/chromeos/wm_overview_controller.cc:364: // Called immediately
before a browser is removed from the list
Style: period at the end of comments.

http://codereview.chromium.org/661237/diff/298001/251007#newcode490
chrome/browser/chromeos/wm_overview_controller.cc:490: BrowserListenerVector
old_listeners = listeners_;
How about calling listeners_.swap(old_listeners) instead of copying and then
clearing.

http://codereview.chromium.org/661237/diff/298001/251007#newcode523
chrome/browser/chromeos/wm_overview_controller.cc:523: Source<const
Browser>(*iterator));
Are you registering for all browser windows? If so, you can do AddSources() and
then only have to register/unregister one global notification observer.

http://codereview.chromium.org/661237/diff/298001/251007#newcode529
chrome/browser/chromeos/wm_overview_controller.cc:529: }  // namespace chromeos
Style: blank line above this.

http://codereview.chromium.org/661237/diff/298001/251010
File chrome/browser/chromeos/wm_overview_controller.h (right):

http://codereview.chromium.org/661237/diff/298001/251010#newcode103
chrome/browser/chromeos/wm_overview_controller.h:103: // Hide the snapshot
windows.
Can you document what the parameter means?

http://codereview.chromium.org/661237/diff/298001/251011
File chrome/browser/chromeos/wm_overview_snapshot.cc (right):

http://codereview.chromium.org/661237/diff/298001/251011#newcode28
chrome/browser/chromeos/wm_overview_snapshot.cc:28:
WmOverviewSnapshot::WmOverviewSnapshot() : WidgetGtk(TYPE_WINDOW),
Style guide says when the initializer list doesn't fit on one line, put the
colon on the next line indented 4 spaces.

http://codereview.chromium.org/661237/diff/298001/251011#newcode78
chrome/browser/chromeos/wm_overview_snapshot.cc:78: }  // end namespace chromeos
Style: put a blank line above this and delete "end"

http://codereview.chromium.org/661237/diff/298001/251009
File chrome/browser/chromeos/wm_overview_snapshot.h (right):

http://codereview.chromium.org/661237/diff/298001/251009#newcode47
chrome/browser/chromeos/wm_overview_snapshot.h:47: int index_;
Can you document what this is the index of?

[Greg Spencer (Chromium), 2010-04-26 19:49:38]

http://codereview.chromium.org/661237/diff/298001/251033
File app/surface/transport_dib.h (right):

http://codereview.chromium.org/661237/diff/298001/251033#newcode79
app/surface/transport_dib.h:79: static Handle TestHandleValue() {
On 2010/04/26 17:21:57, brettw wrote:
> At first I thought this function was "testing" a handle value (like to see if
> it's null or something). How about calling it GetFakeHandleForTest or
something,
> which also makes more clear it's not a real handle value

Done.

http://codereview.chromium.org/661237/diff/298001/251007
File chrome/browser/chromeos/wm_overview_controller.cc (right):

http://codereview.chromium.org/661237/diff/298001/251007#newcode90
chrome/browser/chromeos/wm_overview_controller.cc:90: private:
On 2010/04/26 17:43:48, brettw wrote:
> Style: no blank line after this.

Done.

http://codereview.chromium.org/661237/diff/298001/251007#newcode364
chrome/browser/chromeos/wm_overview_controller.cc:364: // Called immediately
before a browser is removed from the list
On 2010/04/26 17:43:48, brettw wrote:
> Style: period at the end of comments.

Done.

http://codereview.chromium.org/661237/diff/298001/251007#newcode490
chrome/browser/chromeos/wm_overview_controller.cc:490: BrowserListenerVector
old_listeners = listeners_;
On 2010/04/26 17:43:48, brettw wrote:
> How about calling listeners_.swap(old_listeners) instead of copying and then
> clearing.

Done.

http://codereview.chromium.org/661237/diff/298001/251007#newcode523
chrome/browser/chromeos/wm_overview_controller.cc:523: Source<const
Browser>(*iterator));
On 2010/04/26 17:43:48, brettw wrote:
> Are you registering for all browser windows? If so, you can do AddSources()
and
> then only have to register/unregister one global notification observer.

Done.  Thanks, that makes it a lot simpler.

http://codereview.chromium.org/661237/diff/298001/251007#newcode529
chrome/browser/chromeos/wm_overview_controller.cc:529: }  // namespace chromeos
On 2010/04/26 17:43:48, brettw wrote:
> Style: blank line above this.

Fixed.

http://codereview.chromium.org/661237/diff/298001/251010
File chrome/browser/chromeos/wm_overview_controller.h (right):

http://codereview.chromium.org/661237/diff/298001/251010#newcode103
chrome/browser/chromeos/wm_overview_controller.h:103: // Hide the snapshot
windows.
On 2010/04/26 17:43:48, brettw wrote:
> Can you document what the parameter means?

Done.

http://codereview.chromium.org/661237/diff/298001/251011
File chrome/browser/chromeos/wm_overview_snapshot.cc (right):

http://codereview.chromium.org/661237/diff/298001/251011#newcode28
chrome/browser/chromeos/wm_overview_snapshot.cc:28:
WmOverviewSnapshot::WmOverviewSnapshot() : WidgetGtk(TYPE_WINDOW),
On 2010/04/26 17:43:48, brettw wrote:
> Style guide says when the initializer list doesn't fit on one line, put the
> colon on the next line indented 4 spaces.

Done.

http://codereview.chromium.org/661237/diff/298001/251009
File chrome/browser/chromeos/wm_overview_snapshot.h (right):

http://codereview.chromium.org/661237/diff/298001/251009#newcode47
chrome/browser/chromeos/wm_overview_snapshot.h:47: int index_;
On 2010/04/26 17:43:48, brettw wrote:
> Can you document what this is the index of?

Done.

http://codereview.chromium.org/661237/diff/298001/251031
File chrome/browser/renderer_host/backing_store_x.cc (right):

http://codereview.chromium.org/661237/diff/298001/251031#newcode349
chrome/browser/renderer_host/backing_store_x.cc:349: *bitmap.getAddr32(x, y) =
src_row[x] | 0xff000000;
On 2010/04/26 17:21:57, brettw wrote:
> Can you put back the thing where we only call getAddr32 at the beginning of
each
> row? I think there is more work involved in this computation, so normally we
> only do it at the beginning of each line.

Done.

http://codereview.chromium.org/661237/diff/298001/251013
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/661237/diff/298001/251013#newcode216
chrome/browser/renderer_host/render_widget_host.h:216: void
PaintAtSize(TransportDIB::Handle dib_handle, gfx::Size size);
On 2010/04/26 17:21:57, brettw wrote:
> We usually pass Size as a const reference.

Done.

http://codereview.chromium.org/661237/diff/298001/251012
File chrome/browser/tab_contents/thumbnail_generator.cc (right):

http://codereview.chromium.org/661237/diff/298001/251012#newcode301
chrome/browser/tab_contents/thumbnail_generator.cc:301:
callback_map_.erase(item);
On 2010/04/26 17:21:57, brettw wrote:
> I think we'll want some handling for the case where a renderer crashed. It
seems
> like the callback should still get called or at least removed from the list. I
> think TabContents::RenderViewGone is the right place to add a call to the
> thumbnail generator for this.

So, it looks like TabContents::RenderViewGone will notify that the tab contents
have been disconnected, so I'm going to listen for TAB_CONTENTS_DISCONNECTED and
remove callbacks based on this.  That should also take care of the case where
tabs are closed, which I realized I probably don't handle either.

http://codereview.chromium.org/661237/diff/298001/251029
File chrome/browser/tab_contents/thumbnail_generator.h (right):

http://codereview.chromium.org/661237/diff/298001/251029#newcode8
chrome/browser/tab_contents/thumbnail_generator.h:8: #include <vector>
On 2010/04/26 17:21:57, brettw wrote:
> These should be in order.

Done.

http://codereview.chromium.org/661237/diff/298001/251029#newcode107
chrome/browser/tab_contents/thumbnail_generator.h:107: Lock callback_map_lock_;
On 2010/04/26 17:21:57, brettw wrote:
> I'm not sure you need this lock, I don't think this object is used anywhere
but
> the UI thread. Is that true?

Yes, I think this is true, actually.  I thought I might be calling it from
multiple threads when I started, but it didn't turn out that way.  Removed.

http://codereview.chromium.org/661237/diff/298001/251019
File chrome/renderer/render_widget.cc (right):

http://codereview.chromium.org/661237/diff/298001/251019#newcode500
chrome/renderer/render_widget.cc:500: if (pending_input_event_ack_.get()) {
On 2010/04/26 17:21:57, brettw wrote:
> You removed the CallDoDeferredUpdate function. There are a number of early
> returns in this function that mean this code no longer gets called. That seems
> like a problem.

Yep, big problem.  Thanks, I thought I was simplifying things, but didn't think
about the early outs.  I reverted those changes.

[brettw, 2010-04-27 05:07:45]

LGTM!