Change PluginInstallImpl to use base::WindowImpl instead of CWindowImpl to re...

Change PluginInstallImpl to use base::WindowImpl instead of CWindowImpl to reduce a dependency on ATL.

BUG=5023
TEST=Uninstall flash.  Visit hulu.com and install the flash plugin.

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

[Peter Kasting, 2009-08-13 18:32:22]

Added Ben to reviewers list for the last question below.

http://codereview.chromium.org/165469/diff/1/6
File base/window_impl.cc (right):

http://codereview.chromium.org/165469/diff/1/6#newcode157
Line 157: return ::IsWindow(GetNativeView());
Nit: If GetNativeView() becomes non-virtual you should just use hwnd_ directly
in all these functions.

http://codereview.chromium.org/165469/diff/1/7
File base/window_impl.h (right):

http://codereview.chromium.org/165469/diff/1/7#newcode21
Line 21: // An interface implemented by classes that use message maps.
What does this buy you?  I don't see you adding it to any subclasses.

http://codereview.chromium.org/165469/diff/1/7#newcode24
Line 24: virtual BOOL ProcessWindowMessage(HWND window, UINT message,
Nit: I'd give each arg its own line (based on my reading of the Google style
guide)

http://codereview.chromium.org/165469/diff/1/7#newcode43
Line 43: virtual void Init(HWND parent, const gfx::Rect& bounds);
Does this need to be virtual?

http://codereview.chromium.org/165469/diff/1/7#newcode46
Line 46: virtual gfx::NativeView GetNativeView() const;
Can you just return HWND directly?  This is all Windows-only code, I don't see a
benefit to using gfx::NativeView here.

Also, does this really need to be virtual?  Do subclasses actually override it? 
If not please make it non-virtual.

Also, if it becomes non-virtual, then I would make this
HWND get_hwnd() const { return hwnd_; }
i.e. inline it, since it's called frequently.

http://codereview.chromium.org/165469/diff/1/7#newcode69
Line 69: BOOL IsWindow() const;
You should check with Ben about adding these.  I believe that removing these
sorts of public accessors was one of his primary goals in removing ATL.

On a separate note, I think you should return bool instead of BOOL; we decided
at the beginning of the project to avoid BOOL as much as possible.

[James Hawkins, 2009-08-13 19:20:07]

http://codereview.chromium.org/165469/diff/1/6
File base/window_impl.cc (right):

http://codereview.chromium.org/165469/diff/1/6#newcode157
Line 157: return ::IsWindow(GetNativeView());
On 2009/08/13 18:32:23, Peter Kasting wrote:
> Nit: If GetNativeView() becomes non-virtual you should just use hwnd_ directly
> in all these functions.

done.

http://codereview.chromium.org/165469/diff/1/7
File base/window_impl.h (right):

http://codereview.chromium.org/165469/diff/1/7#newcode21
Line 21: // An interface implemented by classes that use message maps.
On 2009/08/13 18:32:23, Peter Kasting wrote:
> What does this buy you?  I don't see you adding it to any subclasses.

ProcessWindowMessage is implemented by the BEGIN_MSG_MAP_EX macro.  The first
implementation used BEGIN_MSG_MAP_EX macro, but ProcessWindowMessage wasn't
virtual, so the child classes' message maps weren't being called.  I've added a
comment to this effect.

http://codereview.chromium.org/165469/diff/1/7#newcode24
Line 24: virtual BOOL ProcessWindowMessage(HWND window, UINT message,
On 2009/08/13 18:32:23, Peter Kasting wrote:
> Nit: I'd give each arg its own line (based on my reading of the Google style
> guide)

done.

http://codereview.chromium.org/165469/diff/1/7#newcode43
Line 43: virtual void Init(HWND parent, const gfx::Rect& bounds);
On 2009/08/13 18:32:23, Peter Kasting wrote:
> Does this need to be virtual?

Not now, so I'll take it out.

http://codereview.chromium.org/165469/diff/1/7#newcode46
Line 46: virtual gfx::NativeView GetNativeView() const;
On 2009/08/13 18:32:23, Peter Kasting wrote:
> Can you just return HWND directly?  This is all Windows-only code, I don't see
a
> benefit to using gfx::NativeView here.
> 
> Also, does this really need to be virtual?  Do subclasses actually override
it? 
> If not please make it non-virtual.
> 
> Also, if it becomes non-virtual, then I would make this
> HWND get_hwnd() const { return hwnd_; }
> i.e. inline it, since it's called frequently.

done.

http://codereview.chromium.org/165469/diff/1/7#newcode69
Line 69: BOOL IsWindow() const;
On 2009/08/13 18:32:23, Peter Kasting wrote:
> You should check with Ben about adding these.  I believe that removing these
> sorts of public accessors was one of his primary goals in removing ATL.
> 
> On a separate note, I think you should return bool instead of BOOL; we decided
> at the beginning of the project to avoid BOOL as much as possible.

They're just wrappers around the win32 API calls of the same name, for
convenience (the point of the class).  One of the design decisions that
influenced these changes is that I wanted to change as little code as possible. 
So we're basically matching what we got with CWindowImpl, and the callers of
this class don't need to be changed.  What's the concern about this?

I've switched the BOOLs to bools.

[Peter Kasting, 2009-08-13 19:24:25]

On Thu, Aug 13, 2009 at 12:20 PM, <jhawkins@chromium.org> wrote:

> http://codereview.chromium.org/165469/diff/1/7#newcode69
> Line 69: BOOL IsWindow() const;
> On 2009/08/13 18:32:23, Peter Kasting wrote:
>
>> You should check with Ben about adding these.  I believe that removing
>>
> these
>
>> sorts of public accessors was one of his primary goals in removing
>>
> ATL.
>
> They're just wrappers around the win32 API calls of the same name, for
> convenience (the point of the class).  One of the design decisions that
> influenced these changes is that I wanted to change as little code as
> possible.  So we're basically matching what we got with CWindowImpl, and
> the callers of this class don't need to be changed.  What's the concern
> about this?


I believe Ben wanted to avoid making win32 APIs on a class publicly
accessible to other people, so that you couldn't go doing some win32
operation directly on an arbitrary class you had a handle to, but that
instead the class would expose a narrow set of APIs that were permissible
for others to call.

I think Ben should clarify what his objective is, and what the steps should
be to achieve it.  Perhaps we want to start with what you're doing here and
then later make these non-public.  I don't know.

PK

http://codereview.chromium.org/165469
>

[Ben Goodger (Google), 2009-08-13 21:45:34]

http://codereview.chromium.org/165469/diff/1037/33
File base/window_impl.h (right):

http://codereview.chromium.org/165469/diff/1037/33#newcode54
Line 54: HWND get_hwnd() const { return hwnd_; }
hwnd()?

http://codereview.chromium.org/165469/diff/1037/33#newcode73
Line 73: // Returns TRUE if this is an existing window.
Returns true if the window has been created.

http://codereview.chromium.org/165469/diff/1037/33#newcode83
Line 83: bool SetWindowPos(HWND insert_after, LPCRECT rect, UINT flags) const;
I hate LPCRECT and other typedefs like this. Use const RECT* instead.

http://codereview.chromium.org/165469/diff/1037/33#newcode92
Line 92: HDC BeginPaint(LPPAINTSTRUCT paint) const;
ditto. PAINTSTRUCT*

[Ben Goodger (Google), 2009-08-13 21:54:08]

http://codereview.chromium.org/165469/diff/1037/33
File base/window_impl.h (right):

http://codereview.chromium.org/165469/diff/1037/33#newcode73
Line 73: // Returns TRUE if this is an existing window.
On 2009/08/13 21:45:34, Ben Goodger wrote:
> Returns true if the window has been created.

Actually, Peter reminds me of my hatred for these pass-thrus. People should just
get the hwnd and call the public API method. We should not add all these
functions.

[James Hawkins, 2009-08-13 23:19:12]

On 2009/08/13 21:54:08, Ben Goodger wrote:
> http://codereview.chromium.org/165469/diff/1037/33
> File base/window_impl.h (right):
> 
> http://codereview.chromium.org/165469/diff/1037/33#newcode73
> Line 73: // Returns TRUE if this is an existing window.
> On 2009/08/13 21:45:34, Ben Goodger wrote:
> > Returns true if the window has been created.
> 
> Actually, Peter reminds me of my hatred for these pass-thrus. People should
just
> get the hwnd and call the public API method. We should not add all these
> functions.

Can you guys take a look at patch set 6?  I've removed the pass-thrus and
changed get_hwnd() to hwnd().

[Peter Kasting, 2009-08-13 23:27:35]

Mostly OK, just a few substantive comments.

I ask about the ::s because most of our code avoids them and they add a lot of
visual noise when reading the code.  I would prefer to only use them when
there's ambiguity.

http://codereview.chromium.org/165469/diff/1042/49
File base/window_impl.cc (right):

http://codereview.chromium.org/165469/diff/1042/49#newcode151
Line 151: #if 0
Don't check in #if 0 blocks.  Just remove this.

http://codereview.chromium.org/165469/diff/1042/49#newcode201
Line 201: bool WindowImpl::DestroyWindow() {
Still have this passthru.  I know it's protected, but why have it at all? 
Subclasses can call DestroyWindow(hwnd()) manually if needed.

http://codereview.chromium.org/165469/diff/1042/51
File chrome/browser/views/tab_contents/tab_contents_view_win.cc (right):

http://codereview.chromium.org/165469/diff/1042/51#newcode250
Line 250: ::InvalidateRect(GetNativeView(), NULL, FALSE);
Nit: Is this change still necessary?

http://codereview.chromium.org/165469/diff/1042/46
File webkit/default_plugin/plugin_impl_win.cc (right):

http://codereview.chromium.org/165469/diff/1042/46#newcode171
Line 171: if (!::IsWindow(hwnd()))
Nit: Are these :: additions still necessary?

http://codereview.chromium.org/165469/diff/1042/47
File webkit/default_plugin/plugin_impl_win.h (right):

http://codereview.chromium.org/165469/diff/1042/47#newcode153
Line 153: HWND window() const { return hwnd(); }
Nit: We should eliminate this and have callers use hwnd() directly.

[James Hawkins, 2009-08-13 23:40:22]

http://codereview.chromium.org/165469/diff/1042/49
File base/window_impl.cc (right):

http://codereview.chromium.org/165469/diff/1042/49#newcode151
Line 151: #if 0
On 2009/08/13 23:27:35, Peter Kasting wrote:
> Don't check in #if 0 blocks.  Just remove this.

Done, forgot to remove it.

http://codereview.chromium.org/165469/diff/1042/49#newcode201
Line 201: bool WindowImpl::DestroyWindow() {
On 2009/08/13 23:27:35, Peter Kasting wrote:
> Still have this passthru.  I know it's protected, but why have it at all? 
> Subclasses can call DestroyWindow(hwnd()) manually if needed.

Done.

http://codereview.chromium.org/165469/diff/1042/51
File chrome/browser/views/tab_contents/tab_contents_view_win.cc (right):

http://codereview.chromium.org/165469/diff/1042/51#newcode250
Line 250: ::InvalidateRect(GetNativeView(), NULL, FALSE);
On 2009/08/13 23:27:35, Peter Kasting wrote:
> Nit: Is this change still necessary?

Done.

http://codereview.chromium.org/165469/diff/1042/46
File webkit/default_plugin/plugin_impl_win.cc (right):

http://codereview.chromium.org/165469/diff/1042/46#newcode171
Line 171: if (!::IsWindow(hwnd()))
On 2009/08/13 23:27:35, Peter Kasting wrote:
> Nit: Are these :: additions still necessary?

I've removed all :: in the file, as they shouldn't be necessary.

http://codereview.chromium.org/165469/diff/1042/47
File webkit/default_plugin/plugin_impl_win.h (right):

http://codereview.chromium.org/165469/diff/1042/47#newcode153
Line 153: HWND window() const { return hwnd(); }
On 2009/08/13 23:27:35, Peter Kasting wrote:
> Nit: We should eliminate this and have callers use hwnd() directly.

Done.

[Peter Kasting, 2009-08-13 23:45:02]

As long as things still compile with the functions removed that you've removed,
LGTM.

[James Hawkins, 2009-08-14 00:36:43]

On 2009/08/13 23:45:02, Peter Kasting wrote:
> As long as things still compile with the functions removed that you've
removed,
> LGTM.

Ok, patch set 9 passed the try bots. I'll commit it when the tree opens.

[levin, 2009-08-14 18:41:45]

The re apply LGTM.  Thanks for your patience.