Refactor RenderWidgetHost::set_paint_observer()

Refactor RenderWidgetHost::set_paint_observer()


BUG=None
TEST=Rerun RenderWidgetHostUnitTest.*

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

[DaveMoore, 2010-12-22 22:38:04]

RenderWidgetHost has two different kinds of paint observers. They are set via
set_paint_observer() and set_painting_observer(). The later transfered ownership
to the RWH the former didn't. And neither supported multiple observers.

I've reimplemented the functionality using notifications.

[brettw, 2011-01-03 19:48:30]

Both the paint observer objects in the instant file and the new_tab_ui file seem
like they can just be deleted now and the stuff can move into the main class. In
both cases, the memory semantics are pretty insane (I think this was to work
around the fact that the old observer took ownership), so I think this is worth
cleaning up even though I feel bad about making the extra work for you.
Hopefully it will be fairly straightforward.

Most of the rest of the comments are just style nits.

http://codereview.chromium.org/6010004/diff/2001/chrome/browser/dom_ui/new_ta...
File chrome/browser/dom_ui/new_tab_ui.cc (right):

http://codereview.chromium.org/6010004/diff/2001/chrome/browser/dom_ui/new_ta...
chrome/browser/dom_ui/new_tab_ui.cc:574: : start_(base::TimeTicks::Now()),
Indent these two lines 2 more spaces.

http://codereview.chromium.org/6010004/diff/2001/chrome/browser/dom_ui/new_ta...
chrome/browser/dom_ui/new_tab_ui.cc:584: const NotificationSource& source,
Be sure args are aligned (either indent these to the (, or if they don't fit,
move "type" down to the next line).

http://codereview.chromium.org/6010004/diff/2001/chrome/browser/dom_ui/new_ta...
File chrome/browser/dom_ui/new_tab_ui.h (right):

http://codereview.chromium.org/6010004/diff/2001/chrome/browser/dom_ui/new_ta...
chrome/browser/dom_ui/new_tab_ui.h:101: NotificationRegistrar registrar_;
Can you put a blank line above this?

http://codereview.chromium.org/6010004/diff/2001/chrome/browser/instant/insta...
File chrome/browser/instant/instant_loader.cc (right):

http://codereview.chromium.org/6010004/diff/2001/chrome/browser/instant/insta...
chrome/browser/instant/instant_loader.cc:151: const NotificationSource& source,
Indent these two args to the first one.

http://codereview.chromium.org/6010004/diff/2001/chrome/browser/instant/insta...
chrome/browser/instant/instant_loader.cc:452: void
InstantLoader::PaintObserverImpl::Observe(NotificationType type,
Move this arg to the next line, indented 4 spaces.

http://codereview.chromium.org/6010004/diff/2001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/thumbnail_generator.cc (right):

http://codereview.chromium.org/6010004/diff/2001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/thumbnail_generator.cc:176: RenderWidgetHost*
renderer,
This should fit on the previous line, I think. (Then just indent the "bool" to
be right below it).

[sky, 2011-01-04 17:06:54]

I agree with Brett. You should be able to make TabContentsDelegateImpl handle
observing now and nuke PaintObserverImpl.

  -Scott

http://codereview.chromium.org/6010004/diff/2001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/6010004/diff/2001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_host.h:390: int tag;
Document what tag is.

[DaveMoore, 2011-01-07 19:04:05]

http://codereview.chromium.org/6010004/diff/2001/chrome/browser/instant/insta...
File chrome/browser/instant/instant_loader.cc (right):

http://codereview.chromium.org/6010004/diff/2001/chrome/browser/instant/insta...
chrome/browser/instant/instant_loader.cc:151: const NotificationSource& source,
N/A
On 2011/01/03 19:48:30, brettw wrote:
> Indent these two args to the first one.

http://codereview.chromium.org/6010004/diff/2001/chrome/browser/instant/insta...
chrome/browser/instant/instant_loader.cc:452: void
InstantLoader::PaintObserverImpl::Observe(NotificationType type,
N/A
On 2011/01/03 19:48:30, brettw wrote:
> Move this arg to the next line, indented 4 spaces.

http://codereview.chromium.org/6010004/diff/2001/chrome/browser/renderer_host...
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/6010004/diff/2001/chrome/browser/renderer_host...
chrome/browser/renderer_host/render_widget_host.h:390: int tag;
On 2011/01/04 17:06:54, sky wrote:
> Document what tag is.

Done.

http://codereview.chromium.org/6010004/diff/2001/chrome/browser/tab_contents/...
File chrome/browser/tab_contents/thumbnail_generator.cc (right):

http://codereview.chromium.org/6010004/diff/2001/chrome/browser/tab_contents/...
chrome/browser/tab_contents/thumbnail_generator.cc:176: RenderWidgetHost*
renderer,
On 2011/01/03 19:48:30, brettw wrote:
> This should fit on the previous line, I think. (Then just indent the "bool" to
> be right below it).

Done.

[sky, 2011-01-07 21:04:15]

The InstantLoader changes LGTM to me after the following are fixed.

http://codereview.chromium.org/6010004/diff/12001/chrome/browser/instant/inst...
File chrome/browser/instant/instant_loader.cc (right):

http://codereview.chromium.org/6010004/diff/12001/chrome/browser/instant/inst...
chrome/browser/instant/instant_loader.cc:227: void
RegisterForPaintNotifications(RenderWidgetHost* render_widget_host) {
Move the register/unregister methods to the private section.

http://codereview.chromium.org/6010004/diff/12001/chrome/browser/instant/inst...
chrome/browser/instant/instant_loader.cc:250: void Observe(NotificationType
type,
virtual

http://codereview.chromium.org/6010004/diff/12001/chrome/browser/instant/inst...
chrome/browser/instant/instant_loader.cc:388: RenderWidgetHost*
registered_render_widget_host_;
Comment?

[brettw, 2011-01-10 19:18:47]

LGTM

http://codereview.chromium.org/6010004/diff/12001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host.h (right):

http://codereview.chromium.org/6010004/diff/12001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host.h:389: struct
PaintAtSizeAckDetails {
Since this is a type, it should go at the top of the public section above the
constructor (where the PaintObserver was before).

[Nico, 2011-01-18 00:39:51]

http://codereview.chromium.org/6010004/diff/12001/chrome/browser/tab_contents...
File chrome/browser/tab_contents/thumbnail_generator.cc (right):

http://codereview.chromium.org/6010004/diff/12001/chrome/browser/tab_contents...
chrome/browser/tab_contents/thumbnail_generator.cc:183: if (monitor |=
currently_monitored) {
Is this supposed to be != (it's currently |=)?

This makes stuff dcheck in Add() with tabpose on mac.