Consider the popup window position when the window shows upward. This patch depends on WebKit patch.

Consider the popup window position when the window shows upward. This patch depends on WebKit patch. https://bugs.webkit.org/show_bug.cgi?id=51382

BUG=60427
TEST= 1. Open attached file 2. Make sure: 2a) the input is at bottom and
window is maximized to make sure the popup appears over the input box
2b) you have more than 1 item to autofill 3. Start typing to filter the
results 4. Listbox appears at top; after filtering items - it
repositions itself so that there's no empty space between the list and
input.

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

[honten.org, 2010-12-21 06:06:01]

I'm now trying to fix the popup window position problem.

This patch also depends on the WebKit side, but I cannot upload even if I use
webkit-patch command.
The error message is following,

https://bugs.webkit.org/show_bug.cgi?id=51378
  Running status to find changed, added, or removed files.
  Reviewing diff to determine which lines changed.
  Extracting affected function names from source files.
  Change author: Naoki Takano <takano.naoki@gmail.com>.
  Description from bug 51378:
    ""[Chromium]  Issue 55874: Auto Fill doesn't preserve case of letters if the
entered text is not from the autofill".
  Editing the WebCore/ChangeLog file.
-- Please remember to include a detailed description in your ChangeLog entry. --
-- See <http://webkit.org/coding/contributing.html> for more info --
Was that diff correct? [Y/n]: Y
Fetching: https://bugs.webkit.org/show_bug.cgi?id=51378&ctype=xml
Failed to run "[u'git', u'svn', u'find-rev', u'r72001']" exit_code: 1 cwd:
/mnt/chrome/src/third_party/WebKit/
Unable to determine upstream SVN information from HEAD history

Do you have any idea?

[honten.org, 2010-12-21 06:39:18]

Sorry, I mistyped BUG number.

The correct number is 60427.

Is there any way to fix the BUG number?

[honten.org, 2010-12-21 07:06:35]

Ok, I can uploaded in 

https://bugs.webkit.org/show_bug.cgi?id=51382

Could you review them?

On 2010/12/21 06:39:18, honten wrote:
> Sorry, I mistyped BUG number.
> 
> The correct number is 60427.
> 
> Is there any way to fix the BUG number?

[honten.org, 2010-12-22 01:06:37]

Is there anybody who can review my change?

Still WebKit review is on going.

On 2010/12/21 07:06:35, honten wrote:
> Ok, I can uploaded in 
> 
> https://bugs.webkit.org/show_bug.cgi?id=51382
> 
> Could you review them?
> 
> On 2010/12/21 06:39:18, honten wrote:
> > Sorry, I mistyped BUG number.
> > 
> > The correct number is 60427.
> > 
> > Is there any way to fix the BUG number?

[Ilya Sherman, 2010-12-22 01:16:35]

+Evan

I don't really know this part of the codebase very well, so I mostly just have
some nits:

http://codereview.chromium.org/6024008/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view.h (right):

http://codereview.chromium.org/6024008/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view.h:93: virtual void
SetMove(const gfx::Point& origin) = 0;
nit: MoveTo() might be a better name for this method

http://codereview.chromium.org/6024008/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view.h:93: virtual void
SetMove(const gfx::Point& origin) = 0;
If you add this method to RenderWidgetHostView, you'll need to add
implementations for all subclasses, not just the Linux one.

http://codereview.chromium.org/6024008/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/6024008/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_gtk.cc:575: origin.x(),
origin.y());
nit: This should be indented to line up with "GTK" above (also in the chunk
below).

[honten.org, 2010-12-22 01:29:41]

Ilya,

Thank you for your review,

> Evan,
Could you review this?

Thanks,

http://codereview.chromium.org/6024008/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view.h (right):

http://codereview.chromium.org/6024008/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view.h:93: virtual void
SetMove(const gfx::Point& origin) = 0;
Yes, you are right.

So once this solution is Ok only in Linux, I'll start implement Mac.

But, as I used to say, I don't have any Windows dev env.
So I might ask somebody to implement Win.

On 2010/12/22 01:16:35, Ilya Sherman wrote:
> If you add this method to RenderWidgetHostView, you'll need to add
> implementations for all subclasses, not just the Linux one.

http://codereview.chromium.org/6024008/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/6024008/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_gtk.cc:575: origin.x(),
origin.y());
Sorry, I cannot understand what you mean.
But I understand this part is not good.

On 2010/12/22 01:16:35, Ilya Sherman wrote:
> nit: This should be indented to line up with "GTK" above (also in the chunk
> below).

[Ilya Sherman, 2010-12-22 01:39:57]

http://codereview.chromium.org/6024008/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/6024008/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_gtk.cc:575: origin.x(),
origin.y());
On 2010/12/22 01:29:41, honten wrote:
> Sorry, I cannot understand what you mean.

This should be formatted like so:

gtk_window_move(GTK_WINDOW(gtk_widget_get_parent(view_.get())),
                origin.x(), origin.y());

[honten.org, 2010-12-22 01:41:27]

http://codereview.chromium.org/6024008/diff/1/chrome/browser/renderer_host/re...
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/6024008/diff/1/chrome/browser/renderer_host/re...
chrome/browser/renderer_host/render_widget_host_view_gtk.cc:575: origin.x(),
origin.y());
Got it,

On 2010/12/22 01:39:58, Ilya Sherman wrote:
> On 2010/12/22 01:29:41, honten wrote:
> > Sorry, I cannot understand what you mean.
> 
> This should be formatted like so:
> 
> gtk_window_move(GTK_WINDOW(gtk_widget_get_parent(view_.get())),
>                 origin.x(), origin.y());

[takano.naoki_gmail.com, 2010-12-22 19:38:18]

Evan,

Could you review my change?

Thanks,

On Tue, Dec 21, 2010 at 5:29 PM,  <Takano.Naoki@gmail.com> wrote:
> Ilya,
>
> Thank you for your review,
>
>> Evan,
>
> Could you review this?
>
> Thanks,
>
>
>
http://codereview.chromium.org/6024008/diff/1/chrome/browser/renderer_host/re...
> File chrome/browser/renderer_host/render_widget_host_view.h (right):
>
>
http://codereview.chromium.org/6024008/diff/1/chrome/browser/renderer_host/re...
> chrome/browser/renderer_host/render_widget_host_view.h:93: virtual void
> SetMove(const gfx::Point& origin) = 0;
> Yes, you are right.
>
> So once this solution is Ok only in Linux, I'll start implement Mac.
>
> But, as I used to say, I don't have any Windows dev env.
> So I might ask somebody to implement Win.
>
> On 2010/12/22 01:16:35, Ilya Sherman wrote:
>>
>> If you add this method to RenderWidgetHostView, you'll need to add
>> implementations for all subclasses, not just the Linux one.
>
>
http://codereview.chromium.org/6024008/diff/1/chrome/browser/renderer_host/re...
> File chrome/browser/renderer_host/render_widget_host_view_gtk.cc
> (right):
>
>
http://codereview.chromium.org/6024008/diff/1/chrome/browser/renderer_host/re...
> chrome/browser/renderer_host/render_widget_host_view_gtk.cc:575:
> origin.x(), origin.y());
> Sorry, I cannot understand what you mean.
> But I understand this part is not good.
>
> On 2010/12/22 01:16:35, Ilya Sherman wrote:
>>
>> nit: This should be indented to line up with "GTK" above (also in the
>
> chunk
>>
>> below).
>
> http://codereview.chromium.org/6024008/
>

[Evan Stade, 2010-12-22 20:41:40]

yes, setmove is a bad name. Where is the webkit side of this patch?

[Evan Stade, 2010-12-22 20:42:09]

you can fix the bug number by pressing the edit issue link on the left of this
page.

[honten.org, 2010-12-22 22:26:50]

Evan,

Thank you for your suggestion.
I fix the bug number.

Also you can find WebKit patch. Please find it.
https://bugs.webkit.org/show_bug.cgi?id=51382

Thanks,

On 2010/12/22 20:42:09, Evan Stade wrote:
> you can fix the bug number by pressing the edit issue link on the left of this
> page.

[honten.org, 2010-12-23 05:14:30]

I update only function name MoveTo.

Also I updated WebKit side,
http://codereview.chromium.org/6024008/

Please review it.

Thanks,

On 2010/12/22 22:26:50, honten wrote:
> Evan,
> 
> Thank you for your suggestion.
> I fix the bug number.
> 
> Also you can find WebKit patch. Please find it.
> https://bugs.webkit.org/show_bug.cgi?id=51382
> 
> Thanks,
> 
> On 2010/12/22 20:42:09, Evan Stade wrote:
> > you can fix the bug number by pressing the edit issue link on the left of
this
> > page.

[honten.org, 2011-01-03 19:05:55]

Could you review my changes?

On 2010/12/23 05:14:30, honten wrote:
> I update only function name MoveTo.
> 
> Also I updated WebKit side,
> http://codereview.chromium.org/6024008/
> 
> Please review it.
> 
> Thanks,
> 
> On 2010/12/22 22:26:50, honten wrote:
> > Evan,
> > 
> > Thank you for your suggestion.
> > I fix the bug number.
> > 
> > Also you can find WebKit patch. Please find it.
> > https://bugs.webkit.org/show_bug.cgi?id=51382
> > 
> > Thanks,
> > 
> > On 2010/12/22 20:42:09, Evan Stade wrote:
> > > you can fix the bug number by pressing the edit issue link on the left of
> this
> > > page.

[honten.org, 2011-01-04 17:40:09]

Evan,

Could you review again?

Thanks,
On 2011/01/03 19:05:55, honten wrote:
> Could you review my changes?
> 
> On 2010/12/23 05:14:30, honten wrote:
> > I update only function name MoveTo.
> > 
> > Also I updated WebKit side,
> > http://codereview.chromium.org/6024008/
> > 
> > Please review it.
> > 
> > Thanks,
> > 
> > On 2010/12/22 22:26:50, honten wrote:
> > > Evan,
> > > 
> > > Thank you for your suggestion.
> > > I fix the bug number.
> > > 
> > > Also you can find WebKit patch. Please find it.
> > > https://bugs.webkit.org/show_bug.cgi?id=51382
> > > 
> > > Thanks,
> > > 
> > > On 2010/12/22 20:42:09, Evan Stade wrote:
> > > > you can fix the bug number by pressing the edit issue link on the left
of
> > this
> > > > page.

[Evan Stade, 2011-01-04 22:49:33]

please put the webkit patch link in the CL description.

I'll hold off on reviewing this until the webkit side is landed.

[takano.naoki_gmail.com, 2011-01-04 23:09:21]

Evan,

Thank you for your comment.
I updated the description.

Ilya,
I updated WebKit patch. I follow your suggestion, but still I have a
couple of questions.
Please read
https://bugs.webkit.org/show_bug.cgi?id=51382

Thanks,

On Tue, Jan 4, 2011 at 2:49 PM,  <estade@chromium.org> wrote:
> please put the webkit patch link in the CL description.
>
> I'll hold off on reviewing this until the webkit side is landed.
>
> http://codereview.chromium.org/6024008/
>

[honten.org, 2011-01-17 10:00:45]

Evan,

Could you review my patch?

Already I got LGTM on webkit side.

I understand this is only for gtk.
Once gtk version is fine, I'll try Mac.

Thanks,

On 2011/01/04 23:09:21, takano.naoki_gmail.com wrote:
> Evan,
> 
> Thank you for your comment.
> I updated the description.
> 
> Ilya,
> I updated WebKit patch. I follow your suggestion, but still I have a
> couple of questions.
> Please read
> https://bugs.webkit.org/show_bug.cgi?id=51382
> 
> Thanks,
> 
> On Tue, Jan 4, 2011 at 2:49 PM,  <mailto:estade@chromium.org> wrote:
> > please put the webkit patch link in the CL description.
> >
> > I'll hold off on reviewing this until the webkit side is landed.
> >
> > http://codereview.chromium.org/6024008/
> >

[honten.org, 2011-01-19 17:42:48]

Is there anybody who can review this part?

As I mentioned, I already landed WebKit patch.

On 2011/01/17 10:00:45, honten wrote:
> Evan,
> 
> Could you review my patch?
> 
> Already I got LGTM on webkit side.
> 
> I understand this is only for gtk.
> Once gtk version is fine, I'll try Mac.
> 
> Thanks,
> 
> On 2011/01/04 23:09:21, http://takano.naoki_gmail.com wrote:
> > Evan,
> > 
> > Thank you for your comment.
> > I updated the description.
> > 
> > Ilya,
> > I updated WebKit patch. I follow your suggestion, but still I have a
> > couple of questions.
> > Please read
> > https://bugs.webkit.org/show_bug.cgi?id=51382
> > 
> > Thanks,
> > 
> > On Tue, Jan 4, 2011 at 2:49 PM,  <mailto:estade@chromium.org> wrote:
> > > please put the webkit patch link in the CL description.
> > >
> > > I'll hold off on reviewing this until the webkit side is landed.
> > >
> > > http://codereview.chromium.org/6024008/
> > >

[honten.org, 2011-01-21 18:41:25]

Ilya,

Do you know who else is familiar with this part?
Evan looks very busy, so I want to ask another person.

As you know, I already landed WebKit patch.

Thanks,

On 2011/01/19 17:42:48, honten wrote:
> Is there anybody who can review this part?
> 
> As I mentioned, I already landed WebKit patch.
> 
> On 2011/01/17 10:00:45, honten wrote:
> > Evan,
> > 
> > Could you review my patch?
> > 
> > Already I got LGTM on webkit side.
> > 
> > I understand this is only for gtk.
> > Once gtk version is fine, I'll try Mac.
> > 
> > Thanks,
> > 
> > On 2011/01/04 23:09:21, http://takano.naoki_gmail.com wrote:
> > > Evan,
> > > 
> > > Thank you for your comment.
> > > I updated the description.
> > > 
> > > Ilya,
> > > I updated WebKit patch. I follow your suggestion, but still I have a
> > > couple of questions.
> > > Please read
> > > https://bugs.webkit.org/show_bug.cgi?id=51382
> > > 
> > > Thanks,
> > > 
> > > On Tue, Jan 4, 2011 at 2:49 PM,  <mailto:estade@chromium.org> wrote:
> > > > please put the webkit patch link in the CL description.
> > > >
> > > > I'll hold off on reviewing this until the webkit side is landed.
> > > >
> > > > http://codereview.chromium.org/6024008/
> > > >

[Evan Stade, 2011-01-22 01:32:05]

http://codereview.chromium.org/6024008/diff/15001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/6024008/diff/15001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_gtk.cc:578: //
TOOLKIT_VIEWS' resize logic flow matches windows. so we go ahead and
are you sure this is necessary?

[honten.org, 2011-01-22 01:53:19]

http://codereview.chromium.org/6024008/diff/15001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/6024008/diff/15001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_gtk.cc:578: //
TOOLKIT_VIEWS' resize logic flow matches windows. so we go ahead and
Actually I'm not sure.

I don't know how to enable TOOLKET_VIEWS, but I mimic it from the prev function.

On 2011/01/22 01:32:05, Evan Stade wrote:
> are you sure this is necessary?

[honten.org, 2011-01-23 04:46:35]

http://codereview.chromium.org/6024008/diff/15001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/6024008/diff/15001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_gtk.cc:578: //
TOOLKIT_VIEWS' resize logic flow matches windows. so we go ahead and
Evan,

I tried to make sure after turning on TOOLKIT_VIES, but I failed to build with
the flag because of link error in the current source code.

Right now, I turned on as following,
 export GYP_DEFINES="toolkit_views=1" 
but still don't know what is difference between enabled and disabled.

Could you tell me?

Thanks,

On 2011/01/22 01:53:19, honten wrote:
> Actually I'm not sure.
> 
> I don't know how to enable TOOLKET_VIEWS, but I mimic it from the prev
function.
> 
> On 2011/01/22 01:32:05, Evan Stade wrote:
> > are you sure this is necessary?
>

[honten.org, 2011-01-23 08:28:54]

How about other part?

Again, I want to make sure this direction is fine or not in GTK.

Once GTK looks Ok, I'll try to fix on Mac with the same way.

Thanks,

http://codereview.chromium.org/6024008/diff/15001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/6024008/diff/15001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_gtk.cc:578: //
TOOLKIT_VIEWS' resize logic flow matches windows. so we go ahead and
Evan,

I made sure we need it.

On 2011/01/23 04:46:35, honten wrote:
> Evan,
> 
> I tried to make sure after turning on TOOLKIT_VIES, but I failed to build with
> the flag because of link error in the current source code.
> 
> Right now, I turned on as following,
>  export GYP_DEFINES="toolkit_views=1" 
> but still don't know what is difference between enabled and disabled.
> 
> Could you tell me?
> 
> Thanks,
> 
> On 2011/01/22 01:53:19, honten wrote:
> > Actually I'm not sure.
> > 
> > I don't know how to enable TOOLKET_VIEWS, but I mimic it from the prev
> function.
> > 
> > On 2011/01/22 01:32:05, Evan Stade wrote:
> > > are you sure this is necessary?
> > 
>

[Evan Stade, 2011-01-24 23:08:02]

http://codereview.chromium.org/6024008/diff/15001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/6024008/diff/15001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_gtk.cc:578: //
TOOLKIT_VIEWS' resize logic flow matches windows. so we go ahead and
On 2011/01/23 08:28:55, honten wrote:
> Evan,
> 
> I made sure we need it.

what does this mean? If you just copied it from the other function, I'm almost
certain we don't need it. The comment talks about sizing the widget. Sizing and
moving are different things.

> 
> On 2011/01/23 04:46:35, honten wrote:
> > Evan,
> > 
> > I tried to make sure after turning on TOOLKIT_VIES, but I failed to build
with
> > the flag because of link error in the current source code.
> > 
> > Right now, I turned on as following,
> >  export GYP_DEFINES="toolkit_views=1" 
> > but still don't know what is difference between enabled and disabled.
> > 
> > Could you tell me?
> > 
> > Thanks,
> > 
> > On 2011/01/22 01:53:19, honten wrote:
> > > Actually I'm not sure.
> > > 
> > > I don't know how to enable TOOLKET_VIEWS, but I mimic it from the prev
> > function.
> > > 
> > > On 2011/01/22 01:32:05, Evan Stade wrote:
> > > > are you sure this is necessary?
> > > 
> > 
>

[takano.naoki_gmail.com, 2011-01-24 23:10:57]

I mean, I made sure with TOOLKIT_VIEWS defined build.
And it didn't effect any non-popup window (e.g. dialog window).
Though it might not be needed... I double-check it.

Except that, what do you think?

On Mon, Jan 24, 2011 at 3:08 PM,  <estade@chromium.org> wrote:
>
>
http://codereview.chromium.org/6024008/diff/15001/chrome/browser/renderer_hos...
> File chrome/browser/renderer_host/render_widget_host_view_gtk.cc
> (right):
>
>
http://codereview.chromium.org/6024008/diff/15001/chrome/browser/renderer_hos...
> chrome/browser/renderer_host/render_widget_host_view_gtk.cc:578: //
> TOOLKIT_VIEWS' resize logic flow matches windows. so we go ahead and
> On 2011/01/23 08:28:55, honten wrote:
>>
>> Evan,
>
>> I made sure we need it.
>
> what does this mean? If you just copied it from the other function, I'm
> almost certain we don't need it. The comment talks about sizing the
> widget. Sizing and moving are different things.
>
>
>> On 2011/01/23 04:46:35, honten wrote:
>> > Evan,
>> >
>> > I tried to make sure after turning on TOOLKIT_VIES, but I failed to
>
> build with
>>
>> > the flag because of link error in the current source code.
>> >
>> > Right now, I turned on as following,
>> >  export GYP_DEFINES="toolkit_views=1"
>> > but still don't know what is difference between enabled and
>
> disabled.
>>
>> >
>> > Could you tell me?
>> >
>> > Thanks,
>> >
>> > On 2011/01/22 01:53:19, honten wrote:
>> > > Actually I'm not sure.
>> > >
>> > > I don't know how to enable TOOLKET_VIEWS, but I mimic it from the
>
> prev
>>
>> > function.
>> > >
>> > > On 2011/01/22 01:32:05, Evan Stade wrote:
>> > > > are you sure this is necessary?
>> > >
>> >
>
>
> http://codereview.chromium.org/6024008/
>

[Evan Stade, 2011-01-24 23:22:39]

aside from that it looks ok, but this code won't compile on all platforms.

[takano.naoki_gmail.com, 2011-01-24 23:24:54]

I guess the error is in test part compilation problem. Sorry, I'll fix it.

Thank you for your dedication every time.

On Mon, Jan 24, 2011 at 3:22 PM,  <estade@chromium.org> wrote:
> aside from that it looks ok, but this code won't compile on all platforms.
>
> http://codereview.chromium.org/6024008/
>

[honten.org, 2011-01-25 06:07:23]

Evan,

The test compilation didn't pass. So I fixed it.

Still I'm working on Mac part.

Thanks,

[honten.org, 2011-01-25 08:21:50]

I implemented Win and Mac.

I confirmed Mac but I'm now sure Win.
Because I don't have Win dev environment.

Is there anybody who can help Win version?

Thanks,

[Evan Stade, 2011-01-25 22:43:29]

avi for mac, pkasting for windows

[Avi (use Gerrit), 2011-01-25 22:52:07]

Comment tweak, but Mac code LGTM.

http://codereview.chromium.org/6024008/diff/39001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/6024008/diff/39001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:627: // upper-left
corner pinned.
You can't really say "pinned" here as that implies "not moving" but this is a
move call. More like, "Do conversions to upper-left origin, as the call
specifies the upper-left point."

[Peter Kasting, 2011-01-25 22:52:19]

I have two comments on the Windows side:

(1) Ben needs to look at this in general.  It worries me that this entire class
seems to do everything with win32 messages and APIs directly and thus we have
SWP_xxx flags and such running around.  We spend a lot of time and effort
getting that stuff right once in Views and I'm not sure why we're not making use
of any of that here.

(2) The new code is basically a copy and paste of the function above.  This kind
of duplication isn't acceptable.  Looking at the GTK and Mac impls as well as
how you're actually calling this I think it makes sense to combine SetSize() and
MoveTo() into a single function that does both functions at once.

[takano.naoki_gmail.com, 2011-01-26 00:31:16]

pkasting,

Thank you for your comment.

About (1), I want to wait Ben's comment.

About (2), I understand what your point.
So do you have any suggestion about the name?

void SetSizeAndMoveTo(const gfx::Size* size, const gfx::Point* point)
is fine?

The reason why I use pointer here is that there might be cases where
caller wants to call size only or point only.

If it's Ok, I'll change it according to that.

Thanks,

On Tue, Jan 25, 2011 at 2:52 PM,  <pkasting@chromium.org> wrote:
> I have two comments on the Windows side:
>
> (1) Ben needs to look at this in general.  It worries me that this entire
> class
> seems to do everything with win32 messages and APIs directly and thus we
> have
> SWP_xxx flags and such running around.  We spend a lot of time and effort
> getting that stuff right once in Views and I'm not sure why we're not making
> use
> of any of that here.
>
> (2) The new code is basically a copy and paste of the function above.  This
> kind
> of duplication isn't acceptable.  Looking at the GTK and Mac impls as well
> as
> how you're actually calling this I think it makes sense to combine SetSize()
> and
> MoveTo() into a single function that does both functions at once.
>
> http://codereview.chromium.org/6024008/
>

[Peter Kasting, 2011-01-26 00:46:53]

On 2011/01/26 00:31:16, takano.naoki_gmail.com wrote:
> About (2), I understand what your point.
> So do you have any suggestion about the name?
> 
> void SetSizeAndMoveTo(const gfx::Size* size, const gfx::Point* point)
> is fine?
> 
> The reason why I use pointer here is that there might be cases where
> caller wants to call size only or point only.

I would do SetBounds(const gfx::Rect&).  This parallels the existing
GetViewBounds() (which honestly ought to be renamed GetBounds()) as well as
other interfaces in our code.

I don't think it's unreasonable to make callers supply a rect.  In the case of
your change in render_widget_host.cc, for example, you already have it.  In
general, if you're a caller that knows enough about the view coordinates to
precisely adjust its size or position, it's fair to say that you're able to
supply both.  This will also make the implementation of the function slightly
simpler.

I think you should also make it very clear what coordinate system you expect
this rect to be in.  It probably makes the most sense to use screen coordinates,
as that's what GetViewBounds() returns (and what SetWindowPos() will expect). 
Even if you choose some other system, you need to document that clearly at the
declaration, which you're not doing now.

BTW, this made me look again at your existing Windows implementation, which
won't work, because you set SWP_NOMOVE, and your coordinates look suspicious as
well.

[takano.naoki_gmail.com, 2011-01-26 01:05:57]

pkasting,

Thank you for your comment and suggestion.

So I'll use SetBounds(), but should I change GetViewBounds() name to
GetBounds() in this change?

Also I have a question about SetWindowsPos()
> I think you should also make it very clear what coordinate system you expect
> this rect to be in.  It probably makes the most sense to use screen
coordinates,
> as that's what GetViewBounds() returns (and what SetWindowPos() will expect).
I agree we should use screen coordinate system, but I guess
SetWindowPos() requires client coordinates, right?

As you see here,
http://msdn.microsoft.com/en-us/library/ms633545(v=vs.85).aspx

The doc says, "The new position of the left side of the window, in
client coordinates."

Or do you mean any other "SetWindowPos()"?

Thanks,

On Tue, Jan 25, 2011 at 4:46 PM,  <pkasting@chromium.org> wrote:
> On 2011/01/26 00:31:16, takano.naoki_gmail.com wrote:
>>
>> About (2), I understand what your point.
>> So do you have any suggestion about the name?
>
>> void SetSizeAndMoveTo(const gfx::Size* size, const gfx::Point* point)
>> is fine?
>
>> The reason why I use pointer here is that there might be cases where
>> caller wants to call size only or point only.
>
> I would do SetBounds(const gfx::Rect&).  This parallels the existing
> GetViewBounds() (which honestly ought to be renamed GetBounds()) as well as
> other interfaces in our code.
>
> I don't think it's unreasonable to make callers supply a rect.  In the case
> of
> your change in render_widget_host.cc, for example, you already have it.  In
> general, if you're a caller that knows enough about the view coordinates to
> precisely adjust its size or position, it's fair to say that you're able to
> supply both.  This will also make the implementation of the function
> slightly
> simpler.
>
> I think you should also make it very clear what coordinate system you expect
> this rect to be in.  It probably makes the most sense to use screen
> coordinates,
> as that's what GetViewBounds() returns (and what SetWindowPos() will
> expect).
> Even if you choose some other system, you need to document that clearly at
> the
> declaration, which you're not doing now.
>
> BTW, this made me look again at your existing Windows implementation, which
> won't work, because you set SWP_NOMOVE, and your coordinates look suspicious
> as
> well.
>
> http://codereview.chromium.org/6024008/
>

[Peter Kasting, 2011-01-26 17:25:04]

On 2011/01/26 01:05:57, takano.naoki_gmail.com wrote:
> So I'll use SetBounds(), but should I change GetViewBounds() name to
> GetBounds() in this change?

Up to you.

> I agree we should use screen coordinate system, but I guess
> SetWindowPos() requires client coordinates, right?
> 
> As you see here,
> http://msdn.microsoft.com/en-us/library/ms633545%28v=vs.85%29.aspx
> 
> The doc says, "The new position of the left side of the window, in
> client coordinates."

If my memory serves, "client coordinates" here means the same thing that you see
in the first paragraph at
http://msdn.microsoft.com/en-us/library/ms633534(v=vs.85).aspx : screen
coordinates for a top-level window, the parent window's client area for a child
window.

[honten.org, 2011-01-29 07:28:25]

Peter,

Still I'm tweaking how to deal with SetBounds().

You suggested that I should make SetBounds() because we already have
GetViewBounds().

GTK and Windows GetViewBounds() returns the screen position and size. So this
fits to our purpose. But Mac's GetViewBounds() is different. Internally this
function just only calls bounds: and the returned origin is always (0,0). So we
cannot use this to get screen position.

More detail, please refer to
http://developer.apple.com/library/mac/#documentation/cocoa/Reference/Applica...:

Also, I suppose, from Mac code, GetViewBounds() implies to call bounds: methods.
If so, the rect position shouldn't be screen coordinate system, right? I'm a
very beginner for Cocoa development. But still I feel a little bit odd.

If we want to get screen window position and to change only size, we can use
RenderWidgetHost::GetWindowRect(). (Actually this function and GetViewBounds()
return the same value on GTK and Windows, though) But we cannot use this
function from Windows because of "#if defined(OS_MAXOSX)", as you know. We can
use this method on Windows, once on we cast to RenderWidgetHostViewWin. But I
guess we shouldn't use the cast.

You told me the example render_widget_host.cc. But actually this is the only one
part where the function changes both pos and size. All other parts change only
sizes.

So I want to leave SetSize() method and add SetBounds(). After that, I SetSize()
call SetBounds() internally.

What do you think?

Thanks,

On 2011/01/26 17:25:04, Peter Kasting wrote:
> On 2011/01/26 01:05:57, http://takano.naoki_gmail.com wrote:
> > So I'll use SetBounds(), but should I change GetViewBounds() name to
> > GetBounds() in this change?
> 
> Up to you.
> 
> > I agree we should use screen coordinate system, but I guess
> > SetWindowPos() requires client coordinates, right?
> > 
> > As you see here,
> > http://msdn.microsoft.com/en-us/library/ms633545%2528v=vs.85%2529.aspx
> > 
> > The doc says, "The new position of the left side of the window, in
> > client coordinates."
> 
> If my memory serves, "client coordinates" here means the same thing that you
see
> in the first paragraph at
> http://msdn.microsoft.com/en-us/library/ms633534%28v=vs.85%29.aspx : screen
> coordinates for a top-level window, the parent window's client area for a
child
> window.

[Peter Kasting, 2011-01-29 19:40:11]

On 2011/01/29 07:28:25, honten wrote:
> GTK and Windows GetViewBounds() returns the screen position and size. So this
> fits to our purpose. But Mac's GetViewBounds() is different. Internally this
> function just only calls bounds: and the returned origin is always (0,0). So
we
> cannot use this to get screen position.

Why can't you change the function to get the screen origin?  Surely that's not
impossible.

It's a bug already if we have a function returning different origins on
different OSes.

> You told me the example render_widget_host.cc. But actually this is the only
one
> part where the function changes both pos and size. All other parts change only
> sizes.

How many other callers are there?

Our code is pretty consistent in other places about using rects rather than
having pairs of setters.

> So I want to leave SetSize() method and add SetBounds(). After that, I
SetSize()
> call SetBounds() internally.

That's probably OK, but it seems like the existing callers of SetSize() should
be audited to ensure they don't really need to be setting the origin (e.g.
because of reasons like the original bug here).

[honten.org, 2011-01-30 22:11:15]

Peter,

Thank you for your quick response.

On 2011/01/29 19:40:11, Peter Kasting wrote:
> On 2011/01/29 07:28:25, honten wrote:
> > GTK and Windows GetViewBounds() returns the screen position and size. So
this
> > fits to our purpose. But Mac's GetViewBounds() is different. Internally this
> > function just only calls bounds: and the returned origin is always (0,0). So
> we
> > cannot use this to get screen position.
> 
> Why can't you change the function to get the screen origin?  Surely that's not
> impossible.
>
> It's a bug already if we have a function returning different origins on
> different OSes.
Ok, so do you mean Mac's GetViewBounds() has a bug? Or GTK's and Windows's have
a bug?

I agree that we have to have consistency among OSes, but I'm not sure which OS
should follow the other OS. Do you have any idea? Or should I ask other people
including Mac experts?

IMHO, GetViewBounds() should return screen coordinate system position. Because
the comments says
// Retrieve the bounds of the View, in screen coordinates.
So Mac's GetViewBounds() should be fixed.

> > You told me the example render_widget_host.cc. But actually this is the only
> one
> > part where the function changes both pos and size. All other parts change
only
> > sizes.
> 
> How many other callers are there?
> 
> Our code is pretty consistent in other places about using rects rather than
> having pairs of setters.
Now 10 caller for SetSize() exist in the source code. render_widget_host.cc part
is only part to change calling both position and size.

I'm not sure the other part using rects or having pairs of setters. But having
pairs is not Chrome way, I'll obey it.

> > So I want to leave SetSize() method and add SetBounds(). After that, I
> SetSize()
> > call SetBounds() internally.
> 
> That's probably OK, but it seems like the existing callers of SetSize() should
> be audited to ensure they don't really need to be setting the origin (e.g.
> because of reasons like the original bug here).
Ok, I'll look into it. But I suppose all existing callers except
render_widget_host.cc should not change the origin because the origin follows
the parent window.

[Peter Kasting, 2011-01-31 00:22:06]

On 2011/01/30 22:11:15, honten wrote:
> IMHO, GetViewBounds() should return screen coordinate system position. Because
> the comments says
> // Retrieve the bounds of the View, in screen coordinates.
> So Mac's GetViewBounds() should be fixed.

Sounds reasonable to me.

> > That's probably OK, but it seems like the existing callers of SetSize()
should
> > be audited to ensure they don't really need to be setting the origin (e.g.
> > because of reasons like the original bug here).
> Ok, I'll look into it. But I suppose all existing callers except
> render_widget_host.cc should not change the origin because the origin follows
> the parent window.

I don't know what sorts of things the existing callers are doing, so I can't
comment.

This is the sort of thing that Ben might be better able to direct than I.  If he
doesn't chime in within the next couple of days, harass him :)

[honten.org, 2011-01-31 00:43:02]

On 2011/01/31 00:22:06, Peter Kasting wrote:
> On 2011/01/30 22:11:15, honten wrote:
> > IMHO, GetViewBounds() should return screen coordinate system position.
Because
> > the comments says
> > // Retrieve the bounds of the View, in screen coordinates.
> > So Mac's GetViewBounds() should be fixed.
> 
> Sounds reasonable to me.
Ok, I'll make sure with Mac guys.

> > > That's probably OK, but it seems like the existing callers of SetSize()
> should
> > > be audited to ensure they don't really need to be setting the origin (e.g.
> > > because of reasons like the original bug here).
> > Ok, I'll look into it. But I suppose all existing callers except
> > render_widget_host.cc should not change the origin because the origin
follows
> > the parent window.
> 
> I don't know what sorts of things the existing callers are doing, so I can't
> comment.
> 
> This is the sort of thing that Ben might be better able to direct than I.  If
he
> doesn't chime in within the next couple of days, harass him :)
Actually, I'm so humble that I don't want to harass him;-)
But I'll ping him again if I cannot get any response within a couple days.

Thanks,

[honten.org, 2011-01-31 01:18:30]

On 2011/01/31 00:43:02, honten wrote:
> On 2011/01/31 00:22:06, Peter Kasting wrote:
> > On 2011/01/30 22:11:15, honten wrote:
> > > IMHO, GetViewBounds() should return screen coordinate system position.
> Because
> > > the comments says
> > > // Retrieve the bounds of the View, in screen coordinates.
> > > So Mac's GetViewBounds() should be fixed.
> > 
> > Sounds reasonable to me.
> Ok, I'll make sure with Mac guys.
I discussed with "thakis" on IRC, and he made an entry
http://code.google.com/p/chromium/issues/detail?id=71368

After this fix, I'll use SetViewFrame() instead of SetBounds().

[Ben Goodger (Google), 2011-02-01 17:57:24]

Addressing Peter's point... despite where it lives in the source tree we have
not thus far considered RWHV to be coupled to views... hence the existing HWND
manipulation. I think it's worthwhile maintaining this distinction... and as we
move TabContents/mparch lower in the directory onion having it be a peer of
ui/views.

I do think this could be simpler however by consolidating these two methods into
one SetBounds call (see my comment below).

http://codereview.chromium.org/6024008/diff/39001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view.h (right):

http://codereview.chromium.org/6024008/diff/39001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view.h:100: virtual void
MoveTo(const gfx::Point& origin) = 0;
How about replacing both of these methods with

virtual void SetBounds(const gfx::Rect& rect) = 0;

[takano.naoki_gmail.com, 2011-02-01 18:29:54]

Ben,

Ben,

Ok, I want to make sure.

So, as I mentioned, there ten caller part of SetSize().
And I guess the all except one request the size, and the position
doesn't need to change.

Right now, here, Peter and thakis are discussing the window coordinate
functions.
http://code.google.com/p/chromium/issues/detail?id=71368

If Perter's suggestion is taken, the function name will be GetScreenBounds().
After that, I want to make SetScreenBounds(), because the passed point
into the function is in screen coordinate.

To replace view->SetSize(size), we have to call as following,
gfx::Rect rect = view->GetScreenBounds();
rect.set_size(size);
view->SetScreenBounds(rect);

Is this OK?

Thanks,

On Tue, Feb 1, 2011 at 9:57 AM,  <ben@chromium.org> wrote:
> Addressing Peter's point... despite where it lives in the source tree we
> have
> not thus far considered RWHV to be coupled to views... hence the existing
> HWND
> manipulation. I think it's worthwhile maintaining this distinction... and as
> we
> move TabContents/mparch lower in the directory onion having it be a peer of
> ui/views.
>
> I do think this could be simpler however by consolidating these two methods
> into
> one SetBounds call (see my comment below).
>
>
>
http://codereview.chromium.org/6024008/diff/39001/chrome/browser/renderer_hos...
> File chrome/browser/renderer_host/render_widget_host_view.h (right):
>
>
http://codereview.chromium.org/6024008/diff/39001/chrome/browser/renderer_hos...
> chrome/browser/renderer_host/render_widget_host_view.h:100: virtual void
> MoveTo(const gfx::Point& origin) = 0;
> How about replacing both of these methods with
>
> virtual void SetBounds(const gfx::Rect& rect) = 0;
>
> http://codereview.chromium.org/6024008/
>

[Ben Goodger (Google), 2011-02-01 18:31:03]

Who calls SetSize?

If you don't want to change all the SetSize callers, keep SetSize on the API
and have it be implemented in terms of SetBounds. That's less error prone.

-Ben

On Tue, Feb 1, 2011 at 10:29 AM, Naoki Takano <takano.naoki@gmail.com>wrote:

> Ben,
>
> Ben,
>
> Ok, I want to make sure.
>
> So, as I mentioned, there ten caller part of SetSize().
> And I guess the all except one request the size, and the position
> doesn't need to change.
>
> Right now, here, Peter and thakis are discussing the window coordinate
> functions.
> http://code.google.com/p/chromium/issues/detail?id=71368
>
> If Perter's suggestion is taken, the function name will be
> GetScreenBounds().
> After that, I want to make SetScreenBounds(), because the passed point
> into the function is in screen coordinate.
>
> To replace view->SetSize(size), we have to call as following,
> gfx::Rect rect = view->GetScreenBounds();
> rect.set_size(size);
> view->SetScreenBounds(rect);
>
> Is this OK?
>
> Thanks,
>
> On Tue, Feb 1, 2011 at 9:57 AM,  <ben@chromium.org> wrote:
> > Addressing Peter's point... despite where it lives in the source tree we
> > have
> > not thus far considered RWHV to be coupled to views... hence the existing
> > HWND
> > manipulation. I think it's worthwhile maintaining this distinction... and
> as
> > we
> > move TabContents/mparch lower in the directory onion having it be a peer
> of
> > ui/views.
> >
> > I do think this could be simpler however by consolidating these two
> methods
> > into
> > one SetBounds call (see my comment below).
> >
> >
> >
>
http://codereview.chromium.org/6024008/diff/39001/chrome/browser/renderer_hos...
> > File chrome/browser/renderer_host/render_widget_host_view.h (right):
> >
> >
>
http://codereview.chromium.org/6024008/diff/39001/chrome/browser/renderer_hos...
> > chrome/browser/renderer_host/render_widget_host_view.h:100: virtual void
> > MoveTo(const gfx::Point& origin) = 0;
> > How about replacing both of these methods with
> >
> > virtual void SetBounds(const gfx::Rect& rect) = 0;
> >
> > http://codereview.chromium.org/6024008/
> >
>

[takano.naoki_gmail.com, 2011-02-01 18:47:03]

Ok,

I'll leave SetSize() and SetSize() calls SetBounds() internally.

Thanks,

On Tue, Feb 1, 2011 at 10:30 AM, Ben Goodger (Google) <ben@chromium.org> wrote:
> Who calls SetSize?
> If you don't want to change all the SetSize callers, keep SetSize on the API
> and have it be implemented in terms of SetBounds. That's less error prone.
> -Ben
>
> On Tue, Feb 1, 2011 at 10:29 AM, Naoki Takano <takano.naoki@gmail.com>
> wrote:
>>
>> Ben,
>>
>> Ben,
>>
>> Ok, I want to make sure.
>>
>> So, as I mentioned, there ten caller part of SetSize().
>> And I guess the all except one request the size, and the position
>> doesn't need to change.
>>
>> Right now, here, Peter and thakis are discussing the window coordinate
>> functions.
>> http://code.google.com/p/chromium/issues/detail?id=71368
>>
>> If Perter's suggestion is taken, the function name will be
>> GetScreenBounds().
>> After that, I want to make SetScreenBounds(), because the passed point
>> into the function is in screen coordinate.
>>
>> To replace view->SetSize(size), we have to call as following,
>> gfx::Rect rect = view->GetScreenBounds();
>> rect.set_size(size);
>> view->SetScreenBounds(rect);
>>
>> Is this OK?
>>
>> Thanks,
>>
>> On Tue, Feb 1, 2011 at 9:57 AM,  <ben@chromium.org> wrote:
>> > Addressing Peter's point... despite where it lives in the source tree we
>> > have
>> > not thus far considered RWHV to be coupled to views... hence the
>> > existing
>> > HWND
>> > manipulation. I think it's worthwhile maintaining this distinction...
>> > and as
>> > we
>> > move TabContents/mparch lower in the directory onion having it be a peer
>> > of
>> > ui/views.
>> >
>> > I do think this could be simpler however by consolidating these two
>> > methods
>> > into
>> > one SetBounds call (see my comment below).
>> >
>> >
>> >
>> >
http://codereview.chromium.org/6024008/diff/39001/chrome/browser/renderer_hos...
>> > File chrome/browser/renderer_host/render_widget_host_view.h (right):
>> >
>> >
>> >
http://codereview.chromium.org/6024008/diff/39001/chrome/browser/renderer_hos...
>> > chrome/browser/renderer_host/render_widget_host_view.h:100: virtual void
>> > MoveTo(const gfx::Point& origin) = 0;
>> > How about replacing both of these methods with
>> >
>> > virtual void SetBounds(const gfx::Rect& rect) = 0;
>> >
>> > http://codereview.chromium.org/6024008/
>> >
>
>

[honten.org, 2011-02-04 06:31:10]

thakis,

Do you have any update about 
http://code.google.com/p/chromium/issues/detail?id=71368
?

[Nico, 2011-02-04 07:24:12]

No. Maybe tomorrow.

On Thu, Feb 3, 2011 at 10:31 PM,  <Takano.Naoki@gmail.com> wrote:
> thakis,
>
> Do you have any update about
> http://code.google.com/p/chromium/issues/detail?id=71368
> ?
>
> http://codereview.chromium.org/6024008/
>

[honten.org, 2011-02-08 05:46:24]

thakis,

How about this problem?

On 2011/02/04 07:24:12, Nico wrote:
> No. Maybe tomorrow.
> 
> On Thu, Feb 3, 2011 at 10:31 PM,  <mailto:Takano.Naoki@gmail.com> wrote:
> > thakis,
> >
> > Do you have any update about
> > http://code.google.com/p/chromium/issues/detail?id=71368
> > ?
> >
> > http://codereview.chromium.org/6024008/
> >

[honten.org, 2011-02-11 05:01:49]

Thanks, Nico.

I confirmed.

I restarted ...

On 2011/02/08 05:46:24, honten wrote:
> thakis,
> 
> How about this problem?
> 
> On 2011/02/04 07:24:12, Nico wrote:
> > No. Maybe tomorrow.
> > 
> > On Thu, Feb 3, 2011 at 10:31 PM,  <mailto:Takano.Naoki@gmail.com> wrote:
> > > thakis,
> > >
> > > Do you have any update about
> > > http://code.google.com/p/chromium/issues/detail?id=71368
> > > ?
> > >
> > > http://codereview.chromium.org/6024008/
> > >

[honten.org, 2011-02-12 23:34:20]

According to Nico's change, I revise it.
Actually I don't have any Windows development environment, so I cannot confirm
though.

Please review it again.

[honten.org, 2011-02-15 18:37:06]

Nico,

Could you review Mac part?

On 2011/02/12 23:34:20, honten wrote:
> According to Nico's change, I revise it.
> Actually I don't have any Windows development environment, so I cannot confirm
> though.
> 
> Please review it again.

[honten.org, 2011-02-16 18:08:11]

Peter,

If you have time, could you review Linux part?

Thanks,

On 2011/02/15 18:37:06, honten wrote:
> Nico,
> 
> Could you review Mac part?
> 
> On 2011/02/12 23:34:20, honten wrote:
> > According to Nico's change, I revise it.
> > Actually I don't have any Windows development environment, so I cannot
confirm
> > though.
> > 
> > Please review it again.

[Peter Kasting, 2011-02-16 18:12:40]

On 2011/02/16 18:08:11, honten wrote:
> Peter,
> 
> If you have time, could you review Linux part?

I'm sorry, I don't know GTK so I can't.  Your patch looks like it might do the
wrong thing for non-popups, though, in that it seems to just drop a move request
on the floor.

[Nico, 2011-02-16 18:18:45]

Thank you so much for working on this!

http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view.h (right):

http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view.h:99: // Tells the View to
size and move itself to the specified size and point.
", in screen space."

http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:693: bounds =
[cocoa_view_ convertRect:bounds toView:nil];
is convertSize:toView: enough?

[Nico, 2011-02-16 18:20:03]

Duh, rietveld ate my one interesting comment. Below:

http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:695:
bounds.size.height - global_origin.y;
global_origin and the screen's frame are in screen coordinates (pixels), while
bounds is in view coordinates. All coordinates have to be in the same coordinate
system when you do calculations on them.

(Also, use NSHeight().)

[honten.org, 2011-02-16 18:39:04]

Peter,

Thank you for your comment, even if you are not GTK review.

However, I cannot understand where you are talking about. What do you mean "just
drop a move request on the floor"?

Do you mention Windows side code?
Or do you mean another thing?

Thanks,

On 2011/02/16 18:12:40, Peter Kasting wrote:
> On 2011/02/16 18:08:11, honten wrote:
> > Peter,
> > 
> > If you have time, could you review Linux part?
> 
> I'm sorry, I don't know GTK so I can't.  Your patch looks like it might do the
> wrong thing for non-popups, though, in that it seems to just drop a move
request
> on the floor.

[Peter Kasting, 2011-02-16 18:43:22]

On 2011/02/16 18:39:04, honten wrote:
> However, I cannot understand where you are talking about. What do you mean
"just
> drop a move request on the floor"?

In RenderWidgetHostViewGtk::SetBounds(), if IsPopup() is false, the function
never tries to adjust the window's origin, only its size.

[honten.org, 2011-02-16 18:45:37]

Niko,

Could you give me your thought?

http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:695:
bounds.size.height - global_origin.y;
Here, my intention is |rect| is actually in view coordinates. So calling
covertRect:bounds converts to screen coordinates and they should be in the same
coordinate. I confirmed with Quartz Debug tools.

If I don't call convertRect:bounds, the render view height position becomes
strange in magnified view.

Or do you mean another problem? Or do I misunderstand?

On 2011/02/16 18:20:03, Nico wrote:
> global_origin and the screen's frame are in screen coordinates (pixels), while
> bounds is in view coordinates. All coordinates have to be in the same
coordinate
> system when you do calculations on them.
> 
> (Also, use NSHeight().)

[honten.org, 2011-02-16 18:51:59]

I see,

I'll make sure.

Thanks,

On 2011/02/16 18:43:22, Peter Kasting wrote:
> On 2011/02/16 18:39:04, honten wrote:
> > However, I cannot understand where you are talking about. What do you mean
> "just
> > drop a move request on the floor"?
> 
> In RenderWidgetHostViewGtk::SetBounds(), if IsPopup() is false, the function
> never tries to adjust the window's origin, only its size.

[Nico, 2011-02-16 21:16:01]

Mac parts LG with nits.

http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:695:
bounds.size.height - global_origin.y;
Chances are I misunderstand something.

…ah! So GetViewBounds() now returns screen coordinates, but SetSize() and
SetBounds() still get view coordinates? That's terrible  :-/ Can you file a bug
on me to fix this, and put a "// |rect.size()| is view coordinates,
|rect.origin| is screen coordinates, TODO(thakis): fix, http://crbug.com/XXX"
comment at the top of the function?

But with this assumption, this code here is correct. (please consider using
convertSize:toView: instead of convertRect:toView:, and use NSHeight()).

On 2011/02/16 18:45:37, honten wrote:
> Here, my intention is |rect| is actually in view coordinates. So calling
> covertRect:bounds converts to screen coordinates and they should be in the
same
> coordinate. I confirmed with Quartz Debug tools.
> 
> If I don't call convertRect:bounds, the render view height position becomes
> strange in magnified view.
> 
> Or do you mean another problem? Or do I misunderstand?
> 
> On 2011/02/16 18:20:03, Nico wrote:
> > global_origin and the screen's frame are in screen coordinates (pixels),
while
> > bounds is in view coordinates. All coordinates have to be in the same
> coordinate
> > system when you do calculations on them.
> > 
> > (Also, use NSHeight().)
>

[honten.org, 2011-02-16 21:18:57]

http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:695:
bounds.size.height - global_origin.y;
Sure, I'll file it and fix the nits.

On 2011/02/16 21:16:01, Nico wrote:
> Chances are I misunderstand something.
> 
> …ah! So GetViewBounds() now returns screen coordinates, but SetSize() and
> SetBounds() still get view coordinates? That's terrible  :-/ Can you file a
bug
> on me to fix this, and put a "// |rect.size()| is view coordinates,
> |rect.origin| is screen coordinates, TODO(thakis): fix,
http://crbug.com/XXX%22
> comment at the top of the function?
> 
> But with this assumption, this code here is correct. (please consider using
> convertSize:toView: instead of convertRect:toView:, and use NSHeight()).
> 
> On 2011/02/16 18:45:37, honten wrote:
> > Here, my intention is |rect| is actually in view coordinates. So calling
> > covertRect:bounds converts to screen coordinates and they should be in the
> same
> > coordinate. I confirmed with Quartz Debug tools.
> > 
> > If I don't call convertRect:bounds, the render view height position becomes
> > strange in magnified view.
> > 
> > Or do you mean another problem? Or do I misunderstand?
> > 
> > On 2011/02/16 18:20:03, Nico wrote:
> > > global_origin and the screen's frame are in screen coordinates (pixels),
> while
> > > bounds is in view coordinates. All coordinates have to be in the same
> > coordinate
> > > system when you do calculations on them.
> > > 
> > > (Also, use NSHeight().)
> > 
>

[honten.org, 2011-02-16 21:20:39]

Nico,

BTW, do you know who are the other area's(Win and Gtk) good people to review?

Could you introduce me?

Thanks,

On 2011/02/16 21:18:57, honten wrote:
>
http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
> File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):
> 
>
http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
> chrome/browser/renderer_host/render_widget_host_view_mac.mm:695:
> bounds.size.height - global_origin.y;
> Sure, I'll file it and fix the nits.
> 
> On 2011/02/16 21:16:01, Nico wrote:
> > Chances are I misunderstand something.
> > 
> > …ah! So GetViewBounds() now returns screen coordinates, but SetSize() and
> > SetBounds() still get view coordinates? That's terrible  :-/ Can you file a
> bug
> > on me to fix this, and put a "// |rect.size()| is view coordinates,
> > |rect.origin| is screen coordinates, TODO(thakis): fix,
> http://crbug.com/XXX%2522
> > comment at the top of the function?
> > 
> > But with this assumption, this code here is correct. (please consider using
> > convertSize:toView: instead of convertRect:toView:, and use NSHeight()).
> > 
> > On 2011/02/16 18:45:37, honten wrote:
> > > Here, my intention is |rect| is actually in view coordinates. So calling
> > > covertRect:bounds converts to screen coordinates and they should be in the
> > same
> > > coordinate. I confirmed with Quartz Debug tools.
> > > 
> > > If I don't call convertRect:bounds, the render view height position
becomes
> > > strange in magnified view.
> > > 
> > > Or do you mean another problem? Or do I misunderstand?
> > > 
> > > On 2011/02/16 18:20:03, Nico wrote:
> > > > global_origin and the screen's frame are in screen coordinates (pixels),
> > while
> > > > bounds is in view coordinates. All coordinates have to be in the same
> > > coordinate
> > > > system when you do calculations on them.
> > > > 
> > > > (Also, use NSHeight().)
> > > 
> >

[Nico, 2011-02-16 21:33:22]

estade (already in the reviewer list) can do gtk. ben or pkasting can
do windows.

On Wed, Feb 16, 2011 at 10:20 PM,  <Takano.Naoki@gmail.com> wrote:
> Nico,
>
> BTW, do you know who are the other area's(Win and Gtk) good people to
> review?
>
> Could you introduce me?
>
> Thanks,
>
> On 2011/02/16 21:18:57, honten wrote:
>
>
http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
>>
>> File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):
>
>
>
http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
>>
>> chrome/browser/renderer_host/render_widget_host_view_mac.mm:695:
>> bounds.size.height - global_origin.y;
>> Sure, I'll file it and fix the nits.
>
>> On 2011/02/16 21:16:01, Nico wrote:
>> > Chances are I misunderstand something.
>> >
>> > …ah! So GetViewBounds() now returns screen coordinates, but SetSize()
>> > and
>> > SetBounds() still get view coordinates? That's terrible  :-/ Can you
>> > file a
>> bug
>> > on me to fix this, and put a "// |rect.size()| is view coordinates,
>> > |rect.origin| is screen coordinates, TODO(thakis): fix,
>> http://crbug.com/XXX%2522
>> > comment at the top of the function?
>> >
>> > But with this assumption, this code here is correct. (please consider
>> > using
>> > convertSize:toView: instead of convertRect:toView:, and use NSHeight()).
>> >
>> > On 2011/02/16 18:45:37, honten wrote:
>> > > Here, my intention is |rect| is actually in view coordinates. So
>> > > calling
>> > > covertRect:bounds converts to screen coordinates and they should be in
>> > > the
>> > same
>> > > coordinate. I confirmed with Quartz Debug tools.
>> > >
>> > > If I don't call convertRect:bounds, the render view height position
>
> becomes
>>
>> > > strange in magnified view.
>> > >
>> > > Or do you mean another problem? Or do I misunderstand?
>> > >
>> > > On 2011/02/16 18:20:03, Nico wrote:
>> > > > global_origin and the screen's frame are in screen coordinates
>> > > > (pixels),
>> > while
>> > > > bounds is in view coordinates. All coordinates have to be in the
>> > > > same
>> > > coordinate
>> > > > system when you do calculations on them.
>> > > >
>> > > > (Also, use NSHeight().)
>> > >
>> >
>
>
>
> http://codereview.chromium.org/6024008/
>

[honten.org, 2011-02-16 21:52:52]

Peter,

Could you review Windows part?

As I wrote it, I don't have Windows Dev environment though.

I'll ask estade about Gtk part.

Thanks,

On 2011/02/16 18:43:22, Peter Kasting wrote:
> On 2011/02/16 18:39:04, honten wrote:
> > However, I cannot understand where you are talking about. What do you mean
> "just
> > drop a move request on the floor"?
> 
> In RenderWidgetHostViewGtk::SetBounds(), if IsPopup() is false, the function
> never tries to adjust the window's origin, only its size.

[Peter Kasting, 2011-02-16 22:01:24]

http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_win.cc:384: if
(rect.origin() == org_rect.orgin())
Nit: I don't think you need this conditional.  SWP_NOMOVE is only needed if
you're going to pass "wrong" origin coordinates that need to be ignored.

http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_win.cc:386:
SetWindowPos(NULL, point.x(), point.y(), window_rect.Width(),
You need to change both |point| and |window_rect| to |rect|, and un-capitalize
Width() and Height(), here and below.

[Evan Stade, 2011-02-16 22:03:17]

http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_gtk.cc:563: gfx::Rect rect
= GetViewBounds();
are you sure this works? I think the origin of GetViewBounds is not in screen
coordinates

[Peter Kasting, 2011-02-16 22:04:21]

http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_gtk.cc:563: gfx::Rect rect
= GetViewBounds();
On 2011/02/16 22:03:17, Evan Stade wrote:
> are you sure this works? I think the origin of GetViewBounds is not in screen
> coordinates

It's supposed to be, per the header comments.  The implementation is in this
file below here, so you can double-check it.

[honten.org, 2011-02-17 08:45:37]

estade,

While I'm revising my change, I noticed a couple of things about Gtk stuff.
Please give me your comment.

http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_gtk.cc:563: gfx::Rect rect
= GetViewBounds();
Oops, I was wrong.

After I double-checked it, I noticed GetViewBounds() had a bug in RWHVGtk.

We should use 
  gtk_widget_get_pointer(view_.get(), &x, &y)
instead of
GtkAllocation* alloc = &view_.get()->allocation;
in GetViewBounds().

I'll fix it.

On 2011/02/16 22:49:59, honten wrote:
> Yes, GetViewBounds() actually returns screen coordinate. I confirmed.
> 
> On 2011/02/16 22:04:21, Peter Kasting wrote:
> > On 2011/02/16 22:03:17, Evan Stade wrote:
> > > are you sure this works? I think the origin of GetViewBounds is not in
> screen
> > > coordinates
> > 
> > It's supposed to be, per the header comments.  The implementation is in this
> > file below here, so you can double-check it.
>

http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_gtk.cc:577: } else {
As Peter pointed out, I should move gtk_window_move() after if() else() close.

But only if the parent widget is window, we can call gtk_window_move() as
following,
GtkWidget* parent_widget = gtk_widget_get_parent(view_.get()); 
if (GTK_IS_WINDOW(parent_widget)) 
  gtk_window_move(GTK_WINDOW(parent_widget), rect.x(), rect.y());

[honten.org, 2011-02-18 09:11:26]

Nico,

Please review Mac part again. The reason why I didn't use NSHeight() is compiler
shows an error if I use NSHeight() here. I don't know why ;-(
NSHeight([[[NSScreen screens] objectAtIndex:0] frame])
should be right?

Actually InitAsPopup() uses very similar code, and I replaced to use NSHeight()
here, it shows the same compiler error.


Peter, 

Please review Windows part.


estade,

Please review GTK part.

Thanks,

http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view.h (right):

http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view.h:99: // Tells the View to
size and move itself to the specified size and point.
On 2011/02/16 18:18:45, Nico wrote:
> ", in screen space."

Done.

http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_gtk.cc:563: gfx::Rect rect
= GetViewBounds();
On 2011/02/17 08:45:37, honten wrote:
> Oops, I was wrong.
> 
> After I double-checked it, I noticed GetViewBounds() had a bug in RWHVGtk.
> 
> We should use 
>   gtk_widget_get_pointer(view_.get(), &x, &y)
> instead of
> GtkAllocation* alloc = &view_.get()->allocation;
> in GetViewBounds().
> 
> I'll fix it.
> 
> On 2011/02/16 22:49:59, honten wrote:
> > Yes, GetViewBounds() actually returns screen coordinate. I confirmed.
> > 
> > On 2011/02/16 22:04:21, Peter Kasting wrote:
> > > On 2011/02/16 22:03:17, Evan Stade wrote:
> > > > are you sure this works? I think the origin of GetViewBounds is not in
> > screen
> > > > coordinates
> > > 
> > > It's supposed to be, per the header comments.  The implementation is in
this
> > > file below here, so you can double-check it.
> > 
> 

Done.

http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_gtk.cc:577: } else {
On 2011/02/17 08:45:37, honten wrote:
> As Peter pointed out, I should move gtk_window_move() after if() else() close.
> 
> But only if the parent widget is window, we can call gtk_window_move() as
> following,
> GtkWidget* parent_widget = gtk_widget_get_parent(view_.get()); 
> if (GTK_IS_WINDOW(parent_widget)) 
>   gtk_window_move(GTK_WINDOW(parent_widget), rect.x(), rect.y()); 

Done.

http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_mac.mm (right):

http://codereview.chromium.org/6024008/diff/55001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_mac.mm:695:
bounds.size.height - global_origin.y;
On 2011/02/16 21:16:01, Nico wrote:
> Chances are I misunderstand something.
> 
> …ah! So GetViewBounds() now returns screen coordinates, but SetSize() and
> SetBounds() still get view coordinates? That's terrible  :-/ Can you file a
bug
> on me to fix this, and put a "// |rect.size()| is view coordinates,
> |rect.origin| is screen coordinates, TODO(thakis): fix,
http://crbug.com/XXX%22
> comment at the top of the function?
> 
> But with this assumption, this code here is correct. (please consider using
> convertSize:toView: instead of convertRect:toView:, and use NSHeight()).
> 
> On 2011/02/16 18:45:37, honten wrote:
> > Here, my intention is |rect| is actually in view coordinates. So calling
> > covertRect:bounds converts to screen coordinates and they should be in the
> same
> > coordinate. I confirmed with Quartz Debug tools.
> > 
> > If I don't call convertRect:bounds, the render view height position becomes
> > strange in magnified view.
> > 
> > Or do you mean another problem? Or do I misunderstand?
> > 
> > On 2011/02/16 18:20:03, Nico wrote:
> > > global_origin and the screen's frame are in screen coordinates (pixels),
> while
> > > bounds is in view coordinates. All coordinates have to be in the same
> > coordinate
> > > system when you do calculations on them.
> > > 
> > > (Also, use NSHeight().)
> > 
> 

Done.

[honten.org, 2011-02-22 01:48:18]

Nico,

Please review Mac part.

[honten.org, 2011-02-22 17:40:54]

Peter, 

Please review Windows part.


estade,

Please review GTK part.

Thanks,

On 2011/02/22 01:48:18, honten wrote:
> Nico,
> 
> Please review Mac part.

[Nico, 2011-02-22 17:47:41]

Mac LGTM

[Evan Stade, 2011-02-22 19:56:18]

the answer is no, GetViewBounds() does not return screen coordinates on Linux.
We should only fix it if we really need to because getting screen coordinates
requires an extra round trip to the X server.

http://codereview.chromium.org/6024008/diff/78001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/6024008/diff/78001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_gtk.cc:607:
SetBounds(rect);
this is backwards. SetBounds should use SetSize, not the reverse.

http://codereview.chromium.org/6024008/diff/78001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_gtk.cc:678:
gtk_widget_get_pointer(view_.get(), &x, &y);
I fail to see how the location of the pointer is relevant to anything

[takano.naoki_gmail.com, 2011-02-22 20:07:22]

estade

Thank you for your review.

But, as Peter says, Mac and Win GetViewBounds() already return screen
coordinates.

Is it Ok to leave the different behavior only on Gtk? Or do we have to
change the name of the function? It looks confusing.

Thanks,

On Tue, Feb 22, 2011 at 11:56 AM,  <estade@chromium.org> wrote:
> the answer is no, GetViewBounds() does not return screen coordinates on
> Linux.
> We should only fix it if we really need to because getting screen
> coordinates
> requires an extra round trip to the X server.
>
>
>
http://codereview.chromium.org/6024008/diff/78001/chrome/browser/renderer_hos...
> File chrome/browser/renderer_host/render_widget_host_view_gtk.cc
> (right):
>
>
http://codereview.chromium.org/6024008/diff/78001/chrome/browser/renderer_hos...
> chrome/browser/renderer_host/render_widget_host_view_gtk.cc:607:
> SetBounds(rect);
> this is backwards. SetBounds should use SetSize, not the reverse.
>
>
http://codereview.chromium.org/6024008/diff/78001/chrome/browser/renderer_hos...
> chrome/browser/renderer_host/render_widget_host_view_gtk.cc:678:
> gtk_widget_get_pointer(view_.get(), &x, &y);
> I fail to see how the location of the pointer is relevant to anything
>
> http://codereview.chromium.org/6024008/
>

[honten.org, 2011-02-22 20:22:02]

Peter,

Could you review Win part?
Also, could you give me your thought about GetViewBounds()?

For me, it's very hard to decide whether GetViewBounds() should return  
 screen or window coordinate.

Thanks,

On 2011/02/22 20:07:22, takano.naoki_gmail.com wrote:
> estade
> 
> Thank you for your review.
> 
> But, as Peter says, Mac and Win GetViewBounds() already return screen
> coordinates.
> 
> Is it Ok to leave the different behavior only on Gtk? Or do we have to
> change the name of the function? It looks confusing.
> 
> Thanks,
> 
> On Tue, Feb 22, 2011 at 11:56 AM,  <mailto:estade@chromium.org> wrote:
> > the answer is no, GetViewBounds() does not return screen coordinates on
> > Linux.
> > We should only fix it if we really need to because getting screen
> > coordinates
> > requires an extra round trip to the X server.
> >
> >
> >
>
http://codereview.chromium.org/6024008/diff/78001/chrome/browser/renderer_hos...
> > File chrome/browser/renderer_host/render_widget_host_view_gtk.cc
> > (right):
> >
> >
>
http://codereview.chromium.org/6024008/diff/78001/chrome/browser/renderer_hos...
> > chrome/browser/renderer_host/render_widget_host_view_gtk.cc:607:
> > SetBounds(rect);
> > this is backwards. SetBounds should use SetSize, not the reverse.
> >
> >
>
http://codereview.chromium.org/6024008/diff/78001/chrome/browser/renderer_hos...
> > chrome/browser/renderer_host/render_widget_host_view_gtk.cc:678:
> > gtk_widget_get_pointer(view_.get(), &x, &y);
> > I fail to see how the location of the pointer is relevant to anything
> >
> > http://codereview.chromium.org/6024008/
> >

[Evan Stade, 2011-02-22 20:37:48]

can you give me a reason why it's necessary to change it?

-- Evan Stade


On Tue, Feb 22, 2011 at 12:07 PM, Naoki Takano <takano.naoki@gmail.com>wrote:

> estade
>
> Thank you for your review.
>
> But, as Peter says, Mac and Win GetViewBounds() already return screen
> coordinates.
>
> Is it Ok to leave the different behavior only on Gtk? Or do we have to
> change the name of the function? It looks confusing.
>
> Thanks,
>
> On Tue, Feb 22, 2011 at 11:56 AM,  <estade@chromium.org> wrote:
> > the answer is no, GetViewBounds() does not return screen coordinates on
> > Linux.
> > We should only fix it if we really need to because getting screen
> > coordinates
> > requires an extra round trip to the X server.
> >
> >
> >
>
http://codereview.chromium.org/6024008/diff/78001/chrome/browser/renderer_hos...
> > File chrome/browser/renderer_host/render_widget_host_view_gtk.cc
> > (right):
> >
> >
>
http://codereview.chromium.org/6024008/diff/78001/chrome/browser/renderer_hos...
> > chrome/browser/renderer_host/render_widget_host_view_gtk.cc:607:
> > SetBounds(rect);
> > this is backwards. SetBounds should use SetSize, not the reverse.
> >
> >
>
http://codereview.chromium.org/6024008/diff/78001/chrome/browser/renderer_hos...
> > chrome/browser/renderer_host/render_widget_host_view_gtk.cc:678:
> > gtk_widget_get_pointer(view_.get(), &x, &y);
> > I fail to see how the location of the pointer is relevant to anything
> >
> > http://codereview.chromium.org/6024008/
> >
>

[takano.naoki_gmail.com, 2011-02-22 20:42:30]

estade,

Thank you for your quick response.

Because we don't have consistency between "Mac and Win" and "Gtk".

If it doesn't make sense, I don't change it.

Actually, as you pointed out already, if I rewrite to use SetSize()
from SetBounds(), we don't have to care GetViewBounds() here. Because
we don't have to call GetViewBounds() in SetSize() anymore.

But, again, still the inconsistency remains in GetViewBounds().

What do you think?

Thanks,

On Tue, Feb 22, 2011 at 12:37 PM, Evan Stade <estade@chromium.org> wrote:
> can you give me a reason why it's necessary to change it?
>
> -- Evan Stade
>
>
> On Tue, Feb 22, 2011 at 12:07 PM, Naoki Takano <takano.naoki@gmail.com>
> wrote:
>>
>> estade
>>
>> Thank you for your review.
>>
>> But, as Peter says, Mac and Win GetViewBounds() already return screen
>> coordinates.
>>
>> Is it Ok to leave the different behavior only on Gtk? Or do we have to
>> change the name of the function? It looks confusing.
>>
>> Thanks,
>>
>> On Tue, Feb 22, 2011 at 11:56 AM,  <estade@chromium.org> wrote:
>> > the answer is no, GetViewBounds() does not return screen coordinates on
>> > Linux.
>> > We should only fix it if we really need to because getting screen
>> > coordinates
>> > requires an extra round trip to the X server.
>> >
>> >
>> >
>> >
http://codereview.chromium.org/6024008/diff/78001/chrome/browser/renderer_hos...
>> > File chrome/browser/renderer_host/render_widget_host_view_gtk.cc
>> > (right):
>> >
>> >
>> >
http://codereview.chromium.org/6024008/diff/78001/chrome/browser/renderer_hos...
>> > chrome/browser/renderer_host/render_widget_host_view_gtk.cc:607:
>> > SetBounds(rect);
>> > this is backwards. SetBounds should use SetSize, not the reverse.
>> >
>> >
>> >
http://codereview.chromium.org/6024008/diff/78001/chrome/browser/renderer_hos...
>> > chrome/browser/renderer_host/render_widget_host_view_gtk.cc:678:
>> > gtk_widget_get_pointer(view_.get(), &x, &y);
>> > I fail to see how the location of the pointer is relevant to anything
>> >
>> > http://codereview.chromium.org/6024008/
>> >
>
>

[honten.org, 2011-02-23 01:31:57]

estade,

As you suggested, I change the order of SetSize() and SetBounds().

Please review again.

http://codereview.chromium.org/6024008/diff/78001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/6024008/diff/78001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_gtk.cc:607:
SetBounds(rect);
On 2011/02/22 19:56:19, Evan Stade wrote:
> this is backwards. SetBounds should use SetSize, not the reverse.

Done.

http://codereview.chromium.org/6024008/diff/78001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_gtk.cc:678:
gtk_widget_get_pointer(view_.get(), &x, &y);
Roll back.
On 2011/02/22 19:56:19, Evan Stade wrote:
> I fail to see how the location of the pointer is relevant to anything

[honten.org, 2011-02-23 19:41:05]

Peter,

Could you review Win part?

If you are busy, could you tell me who is the right person to review?

Thanks,

On 2011/02/23 01:31:57, honten wrote:
> estade,
> 
> As you suggested, I change the order of SetSize() and SetBounds().
> 
> Please review again.
> 
>
http://codereview.chromium.org/6024008/diff/78001/chrome/browser/renderer_hos...
> File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):
> 
>
http://codereview.chromium.org/6024008/diff/78001/chrome/browser/renderer_hos...
> chrome/browser/renderer_host/render_widget_host_view_gtk.cc:607:
> SetBounds(rect);
> On 2011/02/22 19:56:19, Evan Stade wrote:
> > this is backwards. SetBounds should use SetSize, not the reverse.
> 
> Done.
> 
>
http://codereview.chromium.org/6024008/diff/78001/chrome/browser/renderer_hos...
> chrome/browser/renderer_host/render_widget_host_view_gtk.cc:678:
> gtk_widget_get_pointer(view_.get(), &x, &y);
> Roll back.
> On 2011/02/22 19:56:19, Evan Stade wrote:
> > I fail to see how the location of the pointer is relevant to anything

[Peter Kasting, 2011-02-23 19:46:14]

http://codereview.chromium.org/6024008/diff/81001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/6024008/diff/81001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_win.cc:383:
SetWindowPos(NULL, rect.x(), rect.y(), rect.width(),
Nit: Wrap as late as possible (before |swp_flags|)

http://codereview.chromium.org/6024008/diff/81001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_win.cc:386:
::SetWindowPos(compositor_host_window_,
Nit: Just collapse these onto as few lines as possible

http://codereview.chromium.org/6024008/diff/81001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_win.h (right):

http://codereview.chromium.org/6024008/diff/81001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_win.h:133: virtual void
SetBounds(const gfx::rect& rect);
rect -> Rect

[honten.org, 2011-02-23 19:51:53]

Peter,

Thanks!!

So except formatting problem, is the logic fine?

On 2011/02/23 19:46:14, Peter Kasting wrote:
>
http://codereview.chromium.org/6024008/diff/81001/chrome/browser/renderer_hos...
> File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):
> 
>
http://codereview.chromium.org/6024008/diff/81001/chrome/browser/renderer_hos...
> chrome/browser/renderer_host/render_widget_host_view_win.cc:383:
> SetWindowPos(NULL, rect.x(), rect.y(), rect.width(),
> Nit: Wrap as late as possible (before |swp_flags|)
> 
>
http://codereview.chromium.org/6024008/diff/81001/chrome/browser/renderer_hos...
> chrome/browser/renderer_host/render_widget_host_view_win.cc:386:
> ::SetWindowPos(compositor_host_window_,
> Nit: Just collapse these onto as few lines as possible
> 
>
http://codereview.chromium.org/6024008/diff/81001/chrome/browser/renderer_hos...
> File chrome/browser/renderer_host/render_widget_host_view_win.h (right):
> 
>
http://codereview.chromium.org/6024008/diff/81001/chrome/browser/renderer_hos...
> chrome/browser/renderer_host/render_widget_host_view_win.h:133: virtual void
> SetBounds(const gfx::rect& rect);
> rect -> Rect

[Peter Kasting, 2011-02-23 19:53:02]

On 2011/02/23 19:51:53, honten wrote:
> So except formatting problem, is the logic fine?

The logic is fine but without the capitalization fix in the header it won't
compile.

[honten.org, 2011-02-23 20:09:02]

Yes, you are right;-)

On 2011/02/23 19:53:02, Peter Kasting wrote:
> On 2011/02/23 19:51:53, honten wrote:
> > So except formatting problem, is the logic fine?
> 
> The logic is fine but without the capitalization fix in the header it won't
> compile.

[Evan Stade, 2011-02-23 20:25:18]

much better thanks

http://codereview.chromium.org/6024008/diff/81001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/6024008/diff/81001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_gtk.cc:630: if
(GTK_IS_WINDOW(parent_widget))
can you use IsPopup() and then do gtk_widget_get_toplevel

[honten.org, 2011-02-23 21:19:22]

Sure!!

On 2011/02/23 20:25:18, Evan Stade wrote:
> much better thanks
> 
>
http://codereview.chromium.org/6024008/diff/81001/chrome/browser/renderer_hos...
> File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):
> 
>
http://codereview.chromium.org/6024008/diff/81001/chrome/browser/renderer_hos...
> chrome/browser/renderer_host/render_widget_host_view_gtk.cc:630: if
> (GTK_IS_WINDOW(parent_widget))
> can you use IsPopup() and then do gtk_widget_get_toplevel

[honten.org, 2011-02-24 08:03:58]

Please review.

http://codereview.chromium.org/6024008/diff/81001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_gtk.cc (right):

http://codereview.chromium.org/6024008/diff/81001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_gtk.cc:630: if
(GTK_IS_WINDOW(parent_widget))
On 2011/02/23 20:25:18, Evan Stade wrote:
> can you use IsPopup() and then do gtk_widget_get_toplevel

Done.

http://codereview.chromium.org/6024008/diff/81001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/6024008/diff/81001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_win.cc:383:
SetWindowPos(NULL, rect.x(), rect.y(), rect.width(),
On 2011/02/23 19:46:14, Peter Kasting wrote:
> Nit: Wrap as late as possible (before |swp_flags|)

Done.

http://codereview.chromium.org/6024008/diff/81001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_win.cc:386:
::SetWindowPos(compositor_host_window_,
On 2011/02/23 19:46:14, Peter Kasting wrote:
> Nit: Just collapse these onto as few lines as possible

Done.

http://codereview.chromium.org/6024008/diff/81001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_win.h (right):

http://codereview.chromium.org/6024008/diff/81001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_win.h:133: virtual void
SetBounds(const gfx::rect& rect);
On 2011/02/23 19:46:14, Peter Kasting wrote:
> rect -> Rect

Done.

[Peter Kasting, 2011-02-24 18:34:21]

LGTM

http://codereview.chromium.org/6024008/diff/87002/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/6024008/diff/87002/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_win.cc:369: gfx::Rect rect
= GetViewBounds();
Nit: I just realized this function could be simpler:

  SetBounds(gfx::Rect(GetViewBounds.origin(), size));

http://codereview.chromium.org/6024008/diff/87002/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_win.cc:382: gfx::Rect
org_rect = GetViewBounds();
Nit: This is unused, delete it

[takano.naoki_gmail.com, 2011-02-24 18:47:21]

Cool,

Thanks!!

On Thu, Feb 24, 2011 at 10:34 AM,  <pkasting@chromium.org> wrote:
> LGTM
>
>
>
http://codereview.chromium.org/6024008/diff/87002/chrome/browser/renderer_hos...
> File chrome/browser/renderer_host/render_widget_host_view_win.cc
> (right):
>
>
http://codereview.chromium.org/6024008/diff/87002/chrome/browser/renderer_hos...
> chrome/browser/renderer_host/render_widget_host_view_win.cc:369:
> gfx::Rect rect = GetViewBounds();
> Nit: I just realized this function could be simpler:
>
>  SetBounds(gfx::Rect(GetViewBounds.origin(), size));
>
>
http://codereview.chromium.org/6024008/diff/87002/chrome/browser/renderer_hos...
> chrome/browser/renderer_host/render_widget_host_view_win.cc:382:
> gfx::Rect org_rect = GetViewBounds();
> Nit: This is unused, delete it
>
> http://codereview.chromium.org/6024008/
>

[honten.org, 2011-02-25 01:15:36]

estade,

Please review Gtk part.

http://codereview.chromium.org/6024008/diff/87002/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/6024008/diff/87002/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_win.cc:369: gfx::Rect rect
= GetViewBounds();
On 2011/02/24 18:34:21, Peter Kasting wrote:
> Nit: I just realized this function could be simpler:
> 
>   SetBounds(gfx::Rect(GetViewBounds.origin(), size));

Done.

http://codereview.chromium.org/6024008/diff/87002/chrome/browser/renderer_hos...
chrome/browser/renderer_host/render_widget_host_view_win.cc:382: gfx::Rect
org_rect = GetViewBounds();
On 2011/02/24 18:34:21, Peter Kasting wrote:
> Nit: This is unused, delete it

Done.

[Evan Stade, 2011-02-25 23:09:51]

gtk lgtm

[takano.naoki_gmail.com, 2011-02-25 23:10:53]

Cool!!

So can anybody commit this patch?

On Fri, Feb 25, 2011 at 3:09 PM,  <estade@chromium.org> wrote:
> gtk lgtm
>
> http://codereview.chromium.org/6024008/
>

[Evan Stade, 2011-02-25 23:16:12]

please re-sync your patch

[takano.naoki_gmail.com, 2011-02-25 23:17:00]

ok,

On Fri, Feb 25, 2011 at 3:16 PM,  <estade@chromium.org> wrote:
> please re-sync your patch
>
> http://codereview.chromium.org/6024008/
>

[honten.org, 2011-02-26 06:38:35]

Please commit.

[honten.org, 2011-02-28 23:32:16]

estade,

Could you commit?

Or is there anybody who can commit?

Thanks,

On 2011/02/26 06:38:35, honten wrote:
> Please commit.

[Evan Stade, 2011-03-01 01:09:26]

running by trybots now

when I uploaded, it said this:
** Presubmit Messages **
|const NSClass*| is wrong, see http://dev.chromium.org/developers/clang-mac

can you figure out why and fix it if it's a real error?

[takano.naoki_gmail.com, 2011-03-01 01:43:53]

Sure,

I'll ask thakis, if I cannot figure it out.

On Mon, Feb 28, 2011 at 5:09 PM,  <estade@chromium.org> wrote:
> running by trybots now
>
> when I uploaded, it said this:
> ** Presubmit Messages **
> |const NSClass*| is wrong, see http://dev.chromium.org/developers/clang-mac
>
> can you figure out why and fix it if it's a real error?
>
> http://codereview.chromium.org/6024008/
>

[Nico, 2011-03-01 01:44:26]

On Tue, Mar 1, 2011 at 1:09 AM,  <estade@chromium.org> wrote:
> running by trybots now
>
> when I uploaded, it said this:
> ** Presubmit Messages **
> |const NSClass*| is wrong, see http://dev.chromium.org/developers/clang-mac
>
> can you figure out why and fix it if it's a real error?

It's not, this is a false positive in rhwvmac.

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

[takano.naoki_gmail.com, 2011-03-01 01:46:16]

Thanks, Nico,

So could you commit?

On Mon, Feb 28, 2011 at 5:44 PM, Nico Weber <thakis@chromium.org> wrote:
> On Tue, Mar 1, 2011 at 1:09 AM,  <estade@chromium.org> wrote:
>> running by trybots now
>>
>> when I uploaded, it said this:
>> ** Presubmit Messages **
>> |const NSClass*| is wrong, see http://dev.chromium.org/developers/clang-mac
>>
>> can you figure out why and fix it if it's a real error?
>
> It's not, this is a false positive in rhwvmac.
>
>>
>> http://codereview.chromium.org/6024008/
>>
>

[Evan Stade, 2011-03-01 01:58:24]

yes, after I fix the compile errors

[takano.naoki_gmail.com, 2011-03-01 02:05:56]

Thanks, estade,

Sadly, I have no clang environment for now.

Thank you for your support again.

On Mon, Feb 28, 2011 at 5:58 PM,  <estade@chromium.org> wrote:
> yes, after I fix the compile errors
>
> http://codereview.chromium.org/6024008/
>

[Evan Stade, 2011-03-01 20:14:27]

ran it twice and had these 2 test failures (on windows) both times:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[takano.naoki_gmail.com, 2011-03-01 20:17:06]

Is this failure only on Windows only?

As you know, I don't have Windows environment for now. I'm now trying
to set up with VS2008 Express, but it is not succeeded though;-(

On Tue, Mar 1, 2011 at 12:14 PM,  <estade@chromium.org> wrote:
> ran it twice and had these 2 test failures (on windows) both times:
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
>
> http://codereview.chromium.org/6024008/
>

[takano.naoki_gmail.com, 2011-03-01 20:21:10]

estade,

Thank you for your support.
I'll ask Peter to look into more.

On Tue, Mar 1, 2011 at 12:16 PM, Naoki Takano <takano.naoki@gmail.com> wrote:
> Is this failure only on Windows only?
>
> As you know, I don't have Windows environment for now. I'm now trying
> to set up with VS2008 Express, but it is not succeeded though;-(
>
> On Tue, Mar 1, 2011 at 12:14 PM,  <estade@chromium.org> wrote:
>> ran it twice and had these 2 test failures (on windows) both times:
>>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
>>
>> http://codereview.chromium.org/6024008/
>>
>

[honten.org, 2011-03-01 20:26:17]

Peter,

estade tried to commit my patch, but it failed, and he said as following,

> ran it twice and had these 2 test failures (on windows) both times:
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

It looks happens only on Windows, but I'm not sure, because I don't have any
Windows environment now.

I'm now trying to setup with VS2008 Express, but I cannot build it yet.

If you have time, could you help me for Windows part? Or could you help me to
setup Windows environment?

Thanks,

[takano.naoki_gmail.com, 2011-03-02 17:58:11]

Now, I can setup  VS2008 Express, and succeeded the build finally!!
I start to look into the problem...

On Tue, Mar 1, 2011 at 12:26 PM,  <Takano.Naoki@gmail.com> wrote:
> Peter,
>
> estade tried to commit my patch, but it failed, and he said as following,
>
>> ran it twice and had these 2 test failures (on windows) both times:
>
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
>
> It looks happens only on Windows, but I'm not sure, because I don't have any
> Windows environment now.
>
> I'm now trying to setup with VS2008 Express, but I cannot build it yet.
>
> If you have time, could you help me for Windows part? Or could you help me
> to
> setup Windows environment?
>
> Thanks,
>
> http://codereview.chromium.org/6024008/
>

[honten.org, 2011-03-05 07:11:17]

Peter,

Could you please review again?

I think I can fix interactive_ui_tests bug.

On 2011/03/02 17:58:11, takano.naoki_gmail.com wrote:
> Now, I can setup  VS2008 Express, and succeeded the build finally!!
> I start to look into the problem...
> 
> On Tue, Mar 1, 2011 at 12:26 PM,  <mailto:Takano.Naoki@gmail.com> wrote:
> > Peter,
> >
> > estade tried to commit my patch, but it failed, and he said as following,
> >
> >> ran it twice and had these 2 test failures (on windows) both times:
> >
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
> >
> > It looks happens only on Windows, but I'm not sure, because I don't have any
> > Windows environment now.
> >
> > I'm now trying to setup with VS2008 Express, but I cannot build it yet.
> >
> > If you have time, could you help me for Windows part? Or could you help me
> > to
> > setup Windows environment?
> >
> > Thanks,
> >
> > http://codereview.chromium.org/6024008/
> >

[honten.org, 2011-03-07 18:05:36]

Peter,

Could you review again?

On 2011/03/05 07:11:17, honten wrote:
> Peter,
> 
> Could you please review again?
> 
> I think I can fix interactive_ui_tests bug.
> 
> On 2011/03/02 17:58:11, http://takano.naoki_gmail.com wrote:
> > Now, I can setup  VS2008 Express, and succeeded the build finally!!
> > I start to look into the problem...
> > 
> > On Tue, Mar 1, 2011 at 12:26 PM,  <mailto:Takano.Naoki@gmail.com> wrote:
> > > Peter,
> > >
> > > estade tried to commit my patch, but it failed, and he said as following,
> > >
> > >> ran it twice and had these 2 test failures (on windows) both times:
> > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
> > >
> > > It looks happens only on Windows, but I'm not sure, because I don't have
any
> > > Windows environment now.
> > >
> > > I'm now trying to setup with VS2008 Express, but I cannot build it yet.
> > >
> > > If you have time, could you help me for Windows part? Or could you help me
> > > to
> > > setup Windows environment?
> > >
> > > Thanks,
> > >
> > > http://codereview.chromium.org/6024008/
> > >

[Peter Kasting, 2011-03-07 19:25:44]

http://codereview.chromium.org/6024008/diff/105001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/6024008/diff/105001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/render_widget_host_view_win.cc:381: // If the style
is not popup, you have to convert the point to client
The MSDN docs for SetWindowPos() say that the supplied X and Y are always
supposed to be in client coordinates.  There shouldn't be anything specific to
WS_POPUP.

http://codereview.chromium.org/6024008/diff/105001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/6024008/diff/105001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host.cc:801: // Note that we ignore
the position.
This comment lies.

Is the change below desirable?

[honten.org, 2011-03-07 19:36:06]

http://codereview.chromium.org/6024008/diff/105001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/6024008/diff/105001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/render_widget_host_view_win.cc:381: // If the style
is not popup, you have to convert the point to client
Yes, I thought so first. But it caused the problem.

Interestingly, ScreenToClient() for WS_POPUP convert positions to (0,0) even if
|rect| screen position is correct.

So I guess WS_POPUP should be special here.

What do you think?

On 2011/03/07 19:25:44, Peter Kasting wrote:
> The MSDN docs for SetWindowPos() say that the supplied X and Y are always
> supposed to be in client coordinates.  There shouldn't be anything specific to
> WS_POPUP.

http://codereview.chromium.org/6024008/diff/105001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/6024008/diff/105001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host.cc:801: // Note that we ignore
the position.
You are right.

We should should remove it.

On 2011/03/07 19:25:44, Peter Kasting wrote:
> This comment lies.
> 
> Is the change below desirable?

[Peter Kasting, 2011-03-07 20:20:15]

http://codereview.chromium.org/6024008/diff/105001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/6024008/diff/105001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/render_widget_host_view_win.cc:381: // If the style
is not popup, you have to convert the point to client
On 2011/03/07 19:36:07, honten wrote:
> Yes, I thought so first. But it caused the problem.
> 
> Interestingly, ScreenToClient() for WS_POPUP convert positions to (0,0) even
if
> |rect| screen position is correct.
> 
> So I guess WS_POPUP should be special here.

That sounds like a bug in ScreenToClient(), maybe.

Special-casing WS_POPUP _anywhere_ seems wrong.  The issue ought to be whether a
window is a child of another window, not what style it has.

You should ask Ben about this stuff, he understands it more than I do.

http://codereview.chromium.org/6024008/diff/105001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/6024008/diff/105001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host.cc:801: // Note that we ignore
the position.
On 2011/03/07 19:36:07, honten wrote:
> You are right.
> 
> We should should remove it.

I'm just really curious.  "Request move" sounds like something that ought to
respect position.  Why were we not respecting it before?  Why is it right to
ignore it now?

[honten.org, 2011-03-07 21:19:50]

Ben,

Could you give me your comment about the relation between ScreenToClient() and
WS_POPUP.

chrome/browser/renderer_host/render_widget_host_view_win.cc:381

As I wrote here, I get (0,0) position if I call ScreenToClient() when the window
is popup.

Do you know why?

http://codereview.chromium.org/6024008/diff/105001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/6024008/diff/105001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/render_widget_host_view_win.cc:381: // If the style
is not popup, you have to convert the point to client
Ok, I ask Ben.

On 2011/03/07 20:20:15, Peter Kasting wrote:
> On 2011/03/07 19:36:07, honten wrote:
> > Yes, I thought so first. But it caused the problem.
> > 
> > Interestingly, ScreenToClient() for WS_POPUP convert positions to (0,0) even
> if
> > |rect| screen position is correct.
> > 
> > So I guess WS_POPUP should be special here.
> 
> That sounds like a bug in ScreenToClient(), maybe.
> 
> Special-casing WS_POPUP _anywhere_ seems wrong.  The issue ought to be whether
a
> window is a child of another window, not what style it has.
> 
> You should ask Ben about this stuff, he understands it more than I do.

http://codereview.chromium.org/6024008/diff/105001/content/browser/renderer_h...
File content/browser/renderer_host/render_widget_host.cc (right):

http://codereview.chromium.org/6024008/diff/105001/content/browser/renderer_h...
content/browser/renderer_host/render_widget_host.cc:801: // Note that we ignore
the position.
I don't know why. AFAIK, |pos| originally doesn't have correct position
information. That has only size info.

I'm not sure who implemented first.

On 2011/03/07 20:20:15, Peter Kasting wrote:
> On 2011/03/07 19:36:07, honten wrote:
> > You are right.
> > 
> > We should should remove it.
> 
> I'm just really curious.  "Request move" sounds like something that ought to
> respect position.  Why were we not respecting it before?  Why is it right to
> ignore it now?

[honten.org, 2011-03-09 01:42:52]

Ben,

Could you give me your comment?

On 2011/03/07 21:19:50, honten wrote:
> Ben,
> 
> Could you give me your comment about the relation between ScreenToClient() and
> WS_POPUP.
> 
> chrome/browser/renderer_host/render_widget_host_view_win.cc:381
> 
> As I wrote here, I get (0,0) position if I call ScreenToClient() when the
window
> is popup.
> 
> Do you know why?
> 
>
http://codereview.chromium.org/6024008/diff/105001/chrome/browser/renderer_ho...
> File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):
> 
>
http://codereview.chromium.org/6024008/diff/105001/chrome/browser/renderer_ho...
> chrome/browser/renderer_host/render_widget_host_view_win.cc:381: // If the
style
> is not popup, you have to convert the point to client
> Ok, I ask Ben.
> 
> On 2011/03/07 20:20:15, Peter Kasting wrote:
> > On 2011/03/07 19:36:07, honten wrote:
> > > Yes, I thought so first. But it caused the problem.
> > > 
> > > Interestingly, ScreenToClient() for WS_POPUP convert positions to (0,0)
even
> > if
> > > |rect| screen position is correct.
> > > 
> > > So I guess WS_POPUP should be special here.
> > 
> > That sounds like a bug in ScreenToClient(), maybe.
> > 
> > Special-casing WS_POPUP _anywhere_ seems wrong.  The issue ought to be
whether
> a
> > window is a child of another window, not what style it has.
> > 
> > You should ask Ben about this stuff, he understands it more than I do.
> 
>
http://codereview.chromium.org/6024008/diff/105001/content/browser/renderer_h...
> File content/browser/renderer_host/render_widget_host.cc (right):
> 
>
http://codereview.chromium.org/6024008/diff/105001/content/browser/renderer_h...
> content/browser/renderer_host/render_widget_host.cc:801: // Note that we
ignore
> the position.
> I don't know why. AFAIK, |pos| originally doesn't have correct position
> information. That has only size info.
> 
> I'm not sure who implemented first.
> 
> On 2011/03/07 20:20:15, Peter Kasting wrote:
> > On 2011/03/07 19:36:07, honten wrote:
> > > You are right.
> > > 
> > > We should should remove it.
> > 
> > I'm just really curious.  "Request move" sounds like something that ought to
> > respect position.  Why were we not respecting it before?  Why is it right to
> > ignore it now?

[Ben Goodger (Google), 2011-03-09 02:30:32]

http://codereview.chromium.org/6024008/diff/105001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/6024008/diff/105001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/render_widget_host_view_win.cc:381: // If the style
is not popup, you have to convert the point to client
Peter is right, this should check for !GetParent(GetNativeView()) instead.

If a window is not WS_CHILD, its position will be in screen coords. Otherwise it
will be in client coords.

[honten.org, 2011-03-09 02:33:03]

http://codereview.chromium.org/6024008/diff/105001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/6024008/diff/105001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/render_widget_host_view_win.cc:381: // If the style
is not popup, you have to convert the point to client
I see.

So, for now, if WS_POPUP is specified, WS_CHILD is not set. That is the reason
why ScreenToClient() returns (0,0).

Ok, I'll change it.

Thanks!!

On 2011/03/09 02:30:32, Ben Goodger wrote:
> Peter is right, this should check for !GetParent(GetNativeView()) instead.
> 
> If a window is not WS_CHILD, its position will be in screen coords. Otherwise
it
> will be in client coords.

[honten.org, 2011-03-10 05:55:48]

Ben,

Could you review win part?

http://codereview.chromium.org/6024008/diff/105001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/6024008/diff/105001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/render_widget_host_view_win.cc:381: // If the style
is not popup, you have to convert the point to client
Ben,

Unfortunately, this is not right. Even if I check GetParent(GetNativeView()), it
fails. I guess even if the window is not WS_CHILD, GetParent(GetNativeView())
returns the parent window, doesn't it?

Because GetParent(GetNativeView()) return the parent window specified when
calling Create(). As you know, in RenderWidgetHostViewWin::InitAsPopup(), we
specify the parent window.
  
So we should check WS_CHILD instead of parent window, right? Actually I made
sure WS_CHILD checkin works fine.

On 2011/03/09 02:33:03, honten wrote:
> I see.
> 
> So, for now, if WS_POPUP is specified, WS_CHILD is not set. That is the reason
> why ScreenToClient() returns (0,0).
> 
> Ok, I'll change it.
> 
> Thanks!!
> 
> On 2011/03/09 02:30:32, Ben Goodger wrote:
> > Peter is right, this should check for !GetParent(GetNativeView()) instead.
> > 
> > If a window is not WS_CHILD, its position will be in screen coords.
Otherwise
> it
> > will be in client coords.
>

[honten.org, 2011-03-11 03:03:49]

Ben,

Could you review?

On 2011/03/10 05:55:48, honten wrote:
> Ben,
> 
> Could you review win part?
> 
>
http://codereview.chromium.org/6024008/diff/105001/chrome/browser/renderer_ho...
> File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):
> 
>
http://codereview.chromium.org/6024008/diff/105001/chrome/browser/renderer_ho...
> chrome/browser/renderer_host/render_widget_host_view_win.cc:381: // If the
style
> is not popup, you have to convert the point to client
> Ben,
> 
> Unfortunately, this is not right. Even if I check GetParent(GetNativeView()),
it
> fails. I guess even if the window is not WS_CHILD, GetParent(GetNativeView())
> returns the parent window, doesn't it?
> 
> Because GetParent(GetNativeView()) return the parent window specified when
> calling Create(). As you know, in RenderWidgetHostViewWin::InitAsPopup(), we
> specify the parent window.
>   
> So we should check WS_CHILD instead of parent window, right? Actually I made
> sure WS_CHILD checkin works fine.
> 
> On 2011/03/09 02:33:03, honten wrote:
> > I see.
> > 
> > So, for now, if WS_POPUP is specified, WS_CHILD is not set. That is the
reason
> > why ScreenToClient() returns (0,0).
> > 
> > Ok, I'll change it.
> > 
> > Thanks!!
> > 
> > On 2011/03/09 02:30:32, Ben Goodger wrote:
> > > Peter is right, this should check for !GetParent(GetNativeView()) instead.
> > > 
> > > If a window is not WS_CHILD, its position will be in screen coords.
> Otherwise
> > it
> > > will be in client coords.
> >

[Ben Goodger (Google), 2011-03-11 03:39:42]

Windows bits LGTM.

http://codereview.chromium.org/6024008/diff/110001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/6024008/diff/110001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/render_widget_host_view_win.cc:403: CPoint
point(rect.x(), rect.y());
POINT... I prefer not to use ATL types.

[honten.org, 2011-03-12 07:47:07]

Ben,

Could you commit my change if the change is fine?

http://codereview.chromium.org/6024008/diff/110001/chrome/browser/renderer_ho...
File chrome/browser/renderer_host/render_widget_host_view_win.cc (right):

http://codereview.chromium.org/6024008/diff/110001/chrome/browser/renderer_ho...
chrome/browser/renderer_host/render_widget_host_view_win.cc:403: CPoint
point(rect.x(), rect.y());
On 2011/03/11 03:39:42, Ben Goodger wrote:
> POINT... I prefer not to use ATL types.

Done.

[Ben Goodger (Google), 2011-03-12 14:54:00]

LGTM

[takano.naoki_gmail.com, 2011-03-13 02:15:33]

Thanks, Ben.

So could you commit?

I'm not commiter yet.

On Sat, Mar 12, 2011 at 6:54 AM,  <ben@chromium.org> wrote:
> LGTM
>
> http://codereview.chromium.org/6024008/
>

[honten.org, 2011-03-14 18:12:58]

Ben,

Could you commit?

On 2011/03/13 02:15:33, takano.naoki_gmail.com wrote:
> Thanks, Ben.
> 
> So could you commit?
> 
> I'm not commiter yet.
> 
> On Sat, Mar 12, 2011 at 6:54 AM,  <mailto:ben@chromium.org> wrote:
> > LGTM
> >
> > http://codereview.chromium.org/6024008/
> >

[Ben Goodger (Google), 2011-03-14 18:39:39]

Can you find someone less busy? I have a lot going on.

:-)

-Ben

On Mon, Mar 14, 2011 at 11:12 AM, <Takano.Naoki@gmail.com> wrote:

> Ben,
>
> Could you commit?
>
>
> On 2011/03/13 02:15:33, takano.naoki_gmail.com wrote:
>
>> Thanks, Ben.
>>
>
>  So could you commit?
>>
>
>  I'm not commiter yet.
>>
>
>  On Sat, Mar 12, 2011 at 6:54 AM,  <mailto:ben@chromium.org> wrote:
>> > LGTM
>> >
>> > http://codereview.chromium.org/6024008/
>> >
>>
>
>
>
> http://codereview.chromium.org/6024008/
>

[Nico, 2011-03-14 18:41:20]

I'll land it, to prove that I never work on anything important.

On Mon, Mar 14, 2011 at 11:39 AM, Ben Goodger (Google) <ben@chromium.org> wrote:
> Can you find someone less busy? I have a lot going on.
> :-)
> -Ben
>
> On Mon, Mar 14, 2011 at 11:12 AM, <Takano.Naoki@gmail.com> wrote:
>>
>> Ben,
>>
>> Could you commit?
>>
>> On 2011/03/13 02:15:33, takano.naoki_gmail.com wrote:
>>>
>>> Thanks, Ben.
>>
>>> So could you commit?
>>
>>> I'm not commiter yet.
>>
>>> On Sat, Mar 12, 2011 at 6:54 AM,  <mailto:ben@chromium.org> wrote:
>>> > LGTM
>>> >
>>> > http://codereview.chromium.org/6024008/
>>> >
>>
>>
>>
>> http://codereview.chromium.org/6024008/
>
>

[takano.naoki_gmail.com, 2011-03-14 18:54:43]

Ben,

Thank you for your taking your time a lot for me!!

Nico,

I appreciate your support!!

On Mon, Mar 14, 2011 at 11:41 AM, Nico Weber <thakis@chromium.org> wrote:
> I'll land it, to prove that I never work on anything important.
>
> On Mon, Mar 14, 2011 at 11:39 AM, Ben Goodger (Google) <ben@chromium.org>
wrote:
>> Can you find someone less busy? I have a lot going on.
>> :-)
>> -Ben
>>
>> On Mon, Mar 14, 2011 at 11:12 AM, <Takano.Naoki@gmail.com> wrote:
>>>
>>> Ben,
>>>
>>> Could you commit?
>>>
>>> On 2011/03/13 02:15:33, takano.naoki_gmail.com wrote:
>>>>
>>>> Thanks, Ben.
>>>
>>>> So could you commit?
>>>
>>>> I'm not commiter yet.
>>>
>>>> On Sat, Mar 12, 2011 at 6:54 AM,  <mailto:ben@chromium.org> wrote:
>>>> > LGTM
>>>> >
>>>> > http://codereview.chromium.org/6024008/
>>>> >
>>>
>>>
>>>
>>> http://codereview.chromium.org/6024008/
>>
>>
>