Fix several painting glitches for toolstrips.

Fix several painting glitches for toolstrips.

* Reset HWND size when HWNDHtmlView changes size. The two should always be in sync.

* Race: We might already have frames when RenderWidget receives a new background, need to set these to transparent too.

* Race: Toolstrip background not always available by the time ExtensionToolstrip is constucted

* Make images line up correctly for toolstrip buttons.

[Erik does not do reviews, 2009-04-27 16:12:40]

LGTM

my comments are all nits

* Reset HWND size when HWNDHtmlView changes size (stolen from Erik's patch).
Without this we would sometimes end up invisible because we never resized the
HWND from 0,0.

Acording to beng, this shouldn't be the case.  Supposedly, the HWND is actually
getting sized properly.  The bug that this code was fixing is that the size is
just never getting passed down to the RenderWidgetHostView and RenderWidget for
their bookkeeping purposes.  As far as I know, the call to SetSize doesn't touch
the HWND.

http://codereview.chromium.org/99042/diff/1/2
File chrome/browser/extensions/extension_view.cc (right):

http://codereview.chromium.org/99042/diff/1/2#newcode83
Line 83: SchedulePaint();
As far as I can tell, this second SchedulePaint isn't necessary (I did rafaelw's
resizer test without it).  I'm not sure if this is because of the SetSize or
something else.

http://codereview.chromium.org/99042/diff/1/2#newcode89
Line 89: SetVisible(false);
Why not do this in the constructor?  Does this get called multiple times?

http://codereview.chromium.org/99042/diff/1/3
File chrome/browser/extensions/extension_view.h (right):

http://codereview.chromium.org/99042/diff/1/3#newcode82
Line 82: private:
indent

http://codereview.chromium.org/99042/diff/1/3#newcode98
Line 98: // What we shoul set the preferred width to once the ExtensionView has
loaded.
typo: shoul->should

http://codereview.chromium.org/99042/diff/1/5
File chrome/browser/views/bookmark_bar_view.cc (right):

http://codereview.chromium.org/99042/diff/1/5#newcode1439
Line 1439: view->view()->SetBackground(toolstrip_background_);
It feels weird to do view->view() here.  Maybe add this as a method on
ExtensionToolstrip?

http://codereview.chromium.org/99042/diff/1/6
File chrome/browser/views/hwnd_html_view.cc (right):

http://codereview.chromium.org/99042/diff/1/6#newcode51
Line 51: // Propagate the new width to RenderWidgetHostView
width -> size

http://codereview.chromium.org/99042/diff/1/6#newcode53
Line 53: current.width() > 0 && current.height() > 0)
Should probably have a comment here about the non-zero check, since its unclear
what's going on here (bug 11053).

http://codereview.chromium.org/99042/diff/1/6#newcode54
Line 54: render_view_host()->view()->SetSize(gfx::Size(width(), height()));
I think this is fine (it's better than being in ExtensionView), but according to
beng, it's not actually the correct fix, so we still need a reference to the
underlying bug (10835).  The interesting bit here is that the HWND is actually
getting sized properly, but that we're not propogating this size change down to
the RenderWidgetHostView (and RenderWidget).  The way this is supposed to happen
on Windows is via responding to a Windows message (OnSize?).  It was unclear
exactly where Ben wanted to handle this.

[Erik does not do reviews, 2009-04-27 18:07:09]

http://codereview.chromium.org/99042/diff/1/6
File chrome/browser/views/hwnd_html_view.cc (right):

http://codereview.chromium.org/99042/diff/1/6#newcode53
Line 53: current.width() > 0 && current.height() > 0)
one other comment here.  The second test is wrong (should be || not &&).  Rather
than use that though, just use current.IsEmpty().

[Aaron Boodman, 2009-04-27 19:12:13]

On 2009/04/27 16:12:40, Erik Kay wrote:
> LGTM
> 
> my comments are all nits
> 
> * Reset HWND size when HWNDHtmlView changes size (stolen from Erik's patch).
> Without this we would sometimes end up invisible because we never resized the
> HWND from 0,0.
> 
> Acording to beng, this shouldn't be the case.  Supposedly, the HWND is
actually
> getting sized properly.  The bug that this code was fixing is that the size is
> just never getting passed down to the RenderWidgetHostView and RenderWidget
for
> their bookkeeping purposes.  As far as I know, the call to SetSize doesn't
touch
> the HWND.

I've updated the description a bit, but I think this is the right fix. 

> http://codereview.chromium.org/99042/diff/1/2
> File chrome/browser/extensions/extension_view.cc (right):
> 
> http://codereview.chromium.org/99042/diff/1/2#newcode83
> Line 83: SchedulePaint();
> As far as I can tell, this second SchedulePaint isn't necessary (I did
rafaelw's
> resizer test without it).  I'm not sure if this is because of the SetSize or
> something else.

The second schedule paint doesn't seem necessary to me either. Paints are
batched anyway, so I think that no matter what you'll only end up with one
paint. I've removed the first. We'll see how it goes.

> http://codereview.chromium.org/99042/diff/1/2#newcode89
> Line 89: SetVisible(false);
> Why not do this in the constructor?  Does this get called multiple times?

I can't access the RenderWidgetHost (and its view) until later than construction
time.

> http://codereview.chromium.org/99042/diff/1/3
> File chrome/browser/extensions/extension_view.h (right):
> 
> http://codereview.chromium.org/99042/diff/1/3#newcode82
> Line 82: private:
> indent
> 
> http://codereview.chromium.org/99042/diff/1/3#newcode98
> Line 98: // What we shoul set the preferred width to once the ExtensionView
has
> loaded.
> typo: shoul->should

Fixed.

> http://codereview.chromium.org/99042/diff/1/5
> File chrome/browser/views/bookmark_bar_view.cc (right):
> 
> http://codereview.chromium.org/99042/diff/1/5#newcode1439
> Line 1439: view->view()->SetBackground(toolstrip_background_);
> It feels weird to do view->view() here.  Maybe add this as a method on
> ExtensionToolstrip?

I am already delegating this stupid call through so many layers, I don't want to
add another. I renamed the variable to "toolstrip", so now it's
"toolstrip->view()" :).

> http://codereview.chromium.org/99042/diff/1/6
> File chrome/browser/views/hwnd_html_view.cc (right):
> 
> http://codereview.chromium.org/99042/diff/1/6#newcode51
> Line 51: // Propagate the new width to RenderWidgetHostView
> width -> size

Done.

> http://codereview.chromium.org/99042/diff/1/6#newcode53
> Line 53: current.width() > 0 && current.height() > 0)
> Should probably have a comment here about the non-zero check, since its
unclear
> what's going on here (bug 11053).

Done.

[Aaron Boodman, 2009-04-27 19:12:31]

On 2009/04/27 18:07:09, Erik Kay wrote:
> http://codereview.chromium.org/99042/diff/1/6
> File chrome/browser/views/hwnd_html_view.cc (right):
> 
> http://codereview.chromium.org/99042/diff/1/6#newcode53
> Line 53: current.width() > 0 && current.height() > 0)
> one other comment here.  The second test is wrong (should be || not &&). 
Rather
> than use that though, just use current.IsEmpty().

Done.

[Erik does not do reviews, 2009-04-27 19:47:30]

LGTM

http://codereview.chromium.org/99042/diff/17/3007
File chrome/browser/views/hwnd_html_view.cc (right):

http://codereview.chromium.org/99042/diff/17/3007#newcode52
Line 52: if (render_view_host() && render_view_host()->view() &&
!current.IsEmpty())
I still think this isn't the correct final fix (but is good for this CL). You
should reference the two bugs that I had originally, so that we remove this code
when the right fixes arrive.

See: http://codereview.chromium.org/96008/diff2/3001:4011/3010 for the original
comments on the left.