Fix bug 145175: [Win] Frameless app windows are not resizable if no draggable region is specified

Fix bug 145175: [Win] Frameless app windows are not resizable if no draggable region is specified

This happens because RWHV is created for the new RVH instead of the existing RVH associated with the web contents when navigating a web contents to extension URL (see http://crbug.com/123007). When ShellWindowViews tries to set the clickthrough region for resizing purpose, it fails due to no RWHV for the existing RVH. The fix is to make the native window do it again when RWHV is available after RVH is updated.

BUG=145175
TEST=Manual test by launching and resizing frameless window without draggable region specified


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

[jeremya, 2012-10-17 22:56:45]

lgtm

[jeremya, 2012-10-17 22:57:33]

http://codereview.chromium.org/11205004/diff/1/chrome/browser/ui/extensions/n...
File chrome/browser/ui/extensions/native_shell_window.h (right):

http://codereview.chromium.org/11205004/diff/1/chrome/browser/ui/extensions/n...
chrome/browser/ui/extensions/native_shell_window.h:41: virtual void
RenderViewHostChanged() {}
It might be better to keep this interface pure, though.

[jianli, 2012-10-17 23:06:06]

On Wed, Oct 17, 2012 at 3:57 PM, <jeremya@chromium.org> wrote:

>
> http://codereview.chromium.**org/11205004/diff/1/chrome/**
>
browser/ui/extensions/native_**shell_window.h<http://codereview.chromium.org/11205004/diff/1/chrome/browser/ui/extensions/native_shell_window.h>
> File chrome/browser/ui/extensions/**native_shell_window.h (right):
>
> http://codereview.chromium.**org/11205004/diff/1/chrome/**
>
browser/ui/extensions/native_**shell_window.h#newcode41<http://codereview.chromium.org/11205004/diff/1/chrome/browser/ui/extensions/native_shell_window.h#newcode41>
> chrome/browser/ui/extensions/**native_shell_window.h:41: virtual void
> RenderViewHostChanged() {}
> It might be better to keep this interface pure, though.
>

I thought about this but since this fix only applies to Windows and we're
going to remove it after the root issue is fixed, I feel it is simpler to
make it non-pure. I will change it to pure virtual since it is better
practice.


>
http://codereview.chromium.**org/11205004/<http://codereview.chromium.org/112...
>

[jeremya, 2012-10-17 23:14:08]

Yeah, it's a bit of a pain to add all the implementations of it everywhere.
I'm not sure whether it's worth it, but having all the methods pure virtual
really says to the intrepid chromium coder, "don't add any functionality to
this class". That's probably obvious anyway though.


On Thu, Oct 18, 2012 at 10:05 AM, Jian Li <jianli@chromium.org> wrote:

>
>
> On Wed, Oct 17, 2012 at 3:57 PM, <jeremya@chromium.org> wrote:
>
>>
>> http://codereview.chromium.**org/11205004/diff/1/chrome/**
>>
browser/ui/extensions/native_**shell_window.h<http://codereview.chromium.org/11205004/diff/1/chrome/browser/ui/extensions/native_shell_window.h>
>> File chrome/browser/ui/extensions/**native_shell_window.h (right):
>>
>> http://codereview.chromium.**org/11205004/diff/1/chrome/**
>>
browser/ui/extensions/native_**shell_window.h#newcode41<http://codereview.chromium.org/11205004/diff/1/chrome/browser/ui/extensions/native_shell_window.h#newcode41>
>> chrome/browser/ui/extensions/**native_shell_window.h:41: virtual void
>> RenderViewHostChanged() {}
>> It might be better to keep this interface pure, though.
>>
>
> I thought about this but since this fix only applies to Windows and we're
> going to remove it after the root issue is fixed, I feel it is simpler to
> make it non-pure. I will change it to pure virtual since it is better
> practice.
>
>
>>
http://codereview.chromium.**org/11205004/<http://codereview.chromium.org/112...
>>
>
>

[jianli, 2012-10-17 23:15:53]

New patch uploaded to make it pure virtual.

On Wed, Oct 17, 2012 at 4:13 PM, Jeremy Apthorp <jeremya@chromium.org>wrote:

> Yeah, it's a bit of a pain to add all the implementations of it
> everywhere. I'm not sure whether it's worth it, but having all the methods
> pure virtual really says to the intrepid chromium coder, "don't add any
> functionality to this class". That's probably obvious anyway though.
>
>
> On Thu, Oct 18, 2012 at 10:05 AM, Jian Li <jianli@chromium.org> wrote:
>
>>
>>
>> On Wed, Oct 17, 2012 at 3:57 PM, <jeremya@chromium.org> wrote:
>>
>>>
>>> http://codereview.chromium.**org/11205004/diff/1/chrome/**
>>>
browser/ui/extensions/native_**shell_window.h<http://codereview.chromium.org/11205004/diff/1/chrome/browser/ui/extensions/native_shell_window.h>
>>> File chrome/browser/ui/extensions/**native_shell_window.h (right):
>>>
>>> http://codereview.chromium.**org/11205004/diff/1/chrome/**
>>>
browser/ui/extensions/native_**shell_window.h#newcode41<http://codereview.chromium.org/11205004/diff/1/chrome/browser/ui/extensions/native_shell_window.h#newcode41>
>>> chrome/browser/ui/extensions/**native_shell_window.h:41: virtual void
>>> RenderViewHostChanged() {}
>>> It might be better to keep this interface pure, though.
>>>
>>
>> I thought about this but since this fix only applies to Windows and we're
>> going to remove it after the root issue is fixed, I feel it is simpler to
>> make it non-pure. I will change it to pure virtual since it is better
>> practice.
>>
>>
>>>
http://codereview.chromium.**org/11205004/<http://codereview.chromium.org/112...
>>>
>>
>>
>

[jianli, 2012-10-22 21:24:42]

Ben, could you please review the patch? Thanks.

[Ben Goodger (Google), 2012-10-23 16:13:35]

lgtm

http://codereview.chromium.org/11205004/diff/10003/chrome/browser/ui/extensio...
File chrome/browser/ui/extensions/native_shell_window.h (right):

http://codereview.chromium.org/11205004/diff/10003/chrome/browser/ui/extensio...
chrome/browser/ui/extensions/native_shell_window.h:38: // Called when the
RenderViewHost is changed.
this line is useless

[commit-bot: I haz the power, 2012-10-23 20:01:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jianli@chromium.org/11205004/27001

[commit-bot: I haz the power, 2012-10-23 22:09:12]

Change committed as 163698