Don't set view_screen_rect_ on RequestMove ACK

Don't set view_screen_rect_ on RequestMove ACK

In https://codereview.chromium.org/1894333002/ I made the OnRequestMoveAck
method reset the view_screen_rect_ variable to the pending rect. This was a
mistake because the move request is on the *window* rect, not the view rect.
Additionally, it's unnecessary since it's the browser's job to send an
UpdateScreenRects message to the renderer with the new view and window bounds.
This only happens for non-popup windows so popups will now store the view/window
rects immediately. This patch removes the reset in the Ack method.

In addition, the windowRect method confusingly returns the view_screen_rect or
pending_window_rect if there's a pending move. I've changed this to return the
window_screen_rect_ and added a viewRect method for callers that actually
expect the view. I also did some cleanup and replaced the redundant
RootWindowRect with windowRect.

BUG=638671
Committed: https://crrev.com/6b08cd23a5bbe4e1d95bb7e0431fc743e6b0180c
Cr-Commit-Position: refs/heads/master@{#423030}

[bokan, 2016-09-13 17:28:29]

Description was changed from

==========
Use window_screen_rect_ with pending_window_rect_

Previously, changing the window bounds would set the pending_window_rect_ but
when the ACK came back from the browser, we would set the view_screen_rect_
using the pending rect. This is wrong since the window rect includes all the
window decorations like the URL bar, title bar, etc. while the view rect does
not.

This CL sets the window_screen_rect_ on the ACK. There was also some confusion
in consumers of the windowRect about what it actually refers to. I've added a
viewRect method and made changes where needed.

BUG=638671
==========

to

==========
Don't set view_screen_rect_ on RequestMove ACK

In https://codereview.chromium.org/1894333002/ I made the OnRequestMoveAck
method reset the view_screen_rect_ variable to the pending rect. This was a
mistake because the move request is on the *window* rect, not the view rect.
Additionally, it's unnecessary since its the browser's job to send an
UpdateScreenRects message to the renderer with the new view and window bounds.
This patch removes the reset in the Ack method.

In addition, the windowRect method confusingly returns the view_screen_rect or
pending_window_rect if there's a pending move. I've changed this to return the
window_screen_rect_ and added a viewRect method for callers that actually
expect the view. I also did some cleanup and replaced the redundant
RootWindowRect with windowRect.

BUG=638671
==========

[bokan, 2016-09-13 17:44:32]

Patchset #2 (id:20001) has been deleted

[bokan, 2016-09-13 17:44:40]

Patchset #1 (id:1) has been deleted

[bokan, 2016-09-13 17:44:55]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-13 17:45:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-13 18:40:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-13 18:40:48]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bokan, 2016-09-15 23:17:54]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-15 23:18:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bokan, 2016-09-15 23:19:01]

Description was changed from

==========
Don't set view_screen_rect_ on RequestMove ACK

In https://codereview.chromium.org/1894333002/ I made the OnRequestMoveAck
method reset the view_screen_rect_ variable to the pending rect. This was a
mistake because the move request is on the *window* rect, not the view rect.
Additionally, it's unnecessary since its the browser's job to send an
UpdateScreenRects message to the renderer with the new view and window bounds.
This patch removes the reset in the Ack method.

In addition, the windowRect method confusingly returns the view_screen_rect or
pending_window_rect if there's a pending move. I've changed this to return the
window_screen_rect_ and added a viewRect method for callers that actually
expect the view. I also did some cleanup and replaced the redundant
RootWindowRect with windowRect.

BUG=638671
==========

to

==========
Don't set view_screen_rect_ on RequestMove ACK

In https://codereview.chromium.org/1894333002/ I made the OnRequestMoveAck
method reset the view_screen_rect_ variable to the pending rect. This was a
mistake because the move request is on the *window* rect, not the view rect.
Additionally, it's unnecessary since it's the browser's job to send an
UpdateScreenRects message to the renderer with the new view and window bounds.
This patch removes the reset in the Ack method.

In addition, the windowRect method confusingly returns the view_screen_rect or
pending_window_rect if there's a pending move. I've changed this to return the
window_screen_rect_ and added a viewRect method for callers that actually
expect the view. I also did some cleanup and replaced the redundant
RootWindowRect with windowRect.

BUG=638671
==========

[bokan, 2016-09-15 23:19:39]

bokan@chromium.org changed reviewers:
+ avi@chromium.org

[bokan, 2016-09-15 23:19:48]

Hi Avi, ptal.

[commit-bot: I haz the power, 2016-09-16 00:41:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-16 00:41:40]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Avi (use Gerrit), 2016-09-16 02:00:02]

avi@chromium.org changed reviewers:
- avi@chromium.org

[Avi (use Gerrit), 2016-09-16 02:00:02]

I have no idea how this code works, sorry.

Can you find other reviewers?

[bokan, 2016-09-16 15:49:32]

On 2016/09/16 02:00:02, Avi wrote:
> I have no idea how this code works, sorry.
> 
> Can you find other reviewers?

Will do, thanks for quick response.

[bokan, 2016-09-19 23:10:20]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-19 23:10:49]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 01:58:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 01:58:39]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[bokan, 2016-09-21 15:30:35]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-21 15:30:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-21 16:49:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-21 16:49:17]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[bokan, 2016-09-30 20:53:06]

Description was changed from

==========
Don't set view_screen_rect_ on RequestMove ACK

In https://codereview.chromium.org/1894333002/ I made the OnRequestMoveAck
method reset the view_screen_rect_ variable to the pending rect. This was a
mistake because the move request is on the *window* rect, not the view rect.
Additionally, it's unnecessary since it's the browser's job to send an
UpdateScreenRects message to the renderer with the new view and window bounds.
This patch removes the reset in the Ack method.

In addition, the windowRect method confusingly returns the view_screen_rect or
pending_window_rect if there's a pending move. I've changed this to return the
window_screen_rect_ and added a viewRect method for callers that actually
expect the view. I also did some cleanup and replaced the redundant
RootWindowRect with windowRect.

BUG=638671
==========

to

==========
Don't set view_screen_rect_ on RequestMove ACK

In https://codereview.chromium.org/1894333002/ I made the OnRequestMoveAck
method reset the view_screen_rect_ variable to the pending rect. This was a
mistake because the move request is on the *window* rect, not the view rect.
Additionally, it's unnecessary since it's the browser's job to send an
UpdateScreenRects message to the renderer with the new view and window bounds.
This only happens for non-popup windows so popups will now store the view/window
rects immediately. This patch removes the reset in the Ack method.

In addition, the windowRect method confusingly returns the view_screen_rect or
pending_window_rect if there's a pending move. I've changed this to return the
window_screen_rect_ and added a viewRect method for callers that actually
expect the view. I also did some cleanup and replaced the redundant
RootWindowRect with windowRect.

BUG=638671
==========

[bokan, 2016-09-30 21:01:54]

The CQ bit was checked by bokan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 21:02:54]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bokan, 2016-09-30 21:08:12]

bokan@chromium.org changed reviewers:
+ sievers@chromium.org

[bokan, 2016-09-30 21:10:20]

Hi sievers@,

Looks like my timing is bad, I just saw you're departing soon :(.

This is related to a patch you reviewed previously, do you have time to do this
before you go? Or do you know who might know about this code? If not, no
worries, I'll find someone else.

Good luck on your next endeavor!

[no sievers, 2016-09-30 21:34:08]

On 2016/09/30 21:10:20, bokan wrote:
> Hi sievers@,
> 
> Looks like my timing is bad, I just saw you're departing soon :(.
> 
> This is related to a patch you reviewed previously, do you have time to do
this
> before you go? Or do you know who might know about this code? If not, no
> worries, I'll find someone else.
> 
> Good luck on your next endeavor!

I actually don't know that code well. I would try somebody from Aura who might
know the menu popup code (poke a bit around in git).
And then have a content owner stamp it later.

[bokan, 2016-09-30 21:43:55]

bokan@chromium.org changed reviewers:
+ tkent@chromium.org

[bokan, 2016-09-30 21:44:40]

Ok, thanks. It seems like no one's touched this code in forever. tkent@, you've
done a bunch of (Blink-side) work related to <select> popups, could you review
this? Know someone who could?

[bokan, 2016-09-30 21:44:49]

bokan@chromium.org changed reviewers:
- sievers@chromium.org

[commit-bot: I haz the power, 2016-09-30 22:52:41]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-30 22:52:42]

Dry run: This issue passed the CQ dry run.

[tkent, 2016-10-03 04:03:41]

tkent@chromium.org changed reviewers:
+ ananta@chromium.org

[tkent, 2016-10-03 04:04:58]

I'm not familiar with non-Blink part of this CL, but it looks reasonable.
third_party/WebKit lgtm.

ananta@ worked on SELECT popup on Aura.

[ananta, 2016-10-04 01:33:16]

content\renderer lgtm

[bokan, 2016-10-04 14:22:15]

bokan@chromium.org changed reviewers:
+ aelias@chromium.org

[bokan, 2016-10-04 14:22:16]

+aelias@ for content/renderer OWNER

[aelias_OOO_until_Jul13, 2016-10-04 21:12:12]

lgtm

[bokan, 2016-10-04 21:31:24]

The CQ bit was checked by bokan@chromium.org

[commit-bot: I haz the power, 2016-10-04 21:32:53]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-04 23:03:25]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-04 23:03:28]

Failed to apply patch for third_party/WebKit/Source/web/ChromeClientImpl.cpp:
While running git apply --index -3 -p1;
  error: patch failed: third_party/WebKit/Source/web/ChromeClientImpl.cpp:584
  Falling back to three-way merge...
  Applied patch to 'third_party/WebKit/Source/web/ChromeClientImpl.cpp' with
conflicts.
  U third_party/WebKit/Source/web/ChromeClientImpl.cpp

Patch:       third_party/WebKit/Source/web/ChromeClientImpl.cpp
Index: third_party/WebKit/Source/web/ChromeClientImpl.cpp
diff --git a/third_party/WebKit/Source/web/ChromeClientImpl.cpp
b/third_party/WebKit/Source/web/ChromeClientImpl.cpp
index
f28521197e3537525b78dc6acf7cb4b530d1fb58..81d92dcc1e24c9ff121fc64acd16e4c3ed045fac
100644
--- a/third_party/WebKit/Source/web/ChromeClientImpl.cpp
+++ b/third_party/WebKit/Source/web/ChromeClientImpl.cpp
@@ -584,9 +584,9 @@ IntRect ChromeClientImpl::viewportToScreen(const IntRect&
rectInViewport, const
 
     if (client) {
         client->convertViewportToWindow(&screenRect);
-        WebRect windowRect = client->windowRect();
-        screenRect.x += windowRect.x;
-        screenRect.y += windowRect.y;
+        WebRect viewRect = client->viewRect();
+        screenRect.x += viewRect.x;
+        screenRect.y += viewRect.y;
     }
 
     return screenRect;

[bokan, 2016-10-04 23:15:49]

The CQ bit was checked by bokan@chromium.org

[bokan, 2016-10-04 23:15:49]

The patchset sent to the CQ was uploaded after l-g-t-m from tkent@chromium.org,
aelias@chromium.org, ananta@chromium.org
Link to the patchset: https://codereview.chromium.org/2333353002/#ps200001
(title: "Rebase")

[commit-bot: I haz the power, 2016-10-04 23:16:19]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-05 00:55:51]

Description was changed from

==========
Don't set view_screen_rect_ on RequestMove ACK

In https://codereview.chromium.org/1894333002/ I made the OnRequestMoveAck
method reset the view_screen_rect_ variable to the pending rect. This was a
mistake because the move request is on the *window* rect, not the view rect.
Additionally, it's unnecessary since it's the browser's job to send an
UpdateScreenRects message to the renderer with the new view and window bounds.
This only happens for non-popup windows so popups will now store the view/window
rects immediately. This patch removes the reset in the Ack method.

In addition, the windowRect method confusingly returns the view_screen_rect or
pending_window_rect if there's a pending move. I've changed this to return the
window_screen_rect_ and added a viewRect method for callers that actually
expect the view. I also did some cleanup and replaced the redundant
RootWindowRect with windowRect.

BUG=638671
==========

to

==========
Don't set view_screen_rect_ on RequestMove ACK

In https://codereview.chromium.org/1894333002/ I made the OnRequestMoveAck
method reset the view_screen_rect_ variable to the pending rect. This was a
mistake because the move request is on the *window* rect, not the view rect.
Additionally, it's unnecessary since it's the browser's job to send an
UpdateScreenRects message to the renderer with the new view and window bounds.
This only happens for non-popup windows so popups will now store the view/window
rects immediately. This patch removes the reset in the Ack method.

In addition, the windowRect method confusingly returns the view_screen_rect or
pending_window_rect if there's a pending move. I've changed this to return the
window_screen_rect_ and added a viewRect method for callers that actually
expect the view. I also did some cleanup and replaced the redundant
RootWindowRect with windowRect.

BUG=638671
==========

[commit-bot: I haz the power, 2016-10-05 00:55:52]

Committed patchset #9 (id:200001)

[commit-bot: I haz the power, 2016-10-05 00:57:58]

Description was changed from

==========
Don't set view_screen_rect_ on RequestMove ACK

In https://codereview.chromium.org/1894333002/ I made the OnRequestMoveAck
method reset the view_screen_rect_ variable to the pending rect. This was a
mistake because the move request is on the *window* rect, not the view rect.
Additionally, it's unnecessary since it's the browser's job to send an
UpdateScreenRects message to the renderer with the new view and window bounds.
This only happens for non-popup windows so popups will now store the view/window
rects immediately. This patch removes the reset in the Ack method.

In addition, the windowRect method confusingly returns the view_screen_rect or
pending_window_rect if there's a pending move. I've changed this to return the
window_screen_rect_ and added a viewRect method for callers that actually
expect the view. I also did some cleanup and replaced the redundant
RootWindowRect with windowRect.

BUG=638671
==========

to

==========
Don't set view_screen_rect_ on RequestMove ACK

In https://codereview.chromium.org/1894333002/ I made the OnRequestMoveAck
method reset the view_screen_rect_ variable to the pending rect. This was a
mistake because the move request is on the *window* rect, not the view rect.
Additionally, it's unnecessary since it's the browser's job to send an
UpdateScreenRects message to the renderer with the new view and window bounds.
This only happens for non-popup windows so popups will now store the view/window
rects immediately. This patch removes the reset in the Ack method.

In addition, the windowRect method confusingly returns the view_screen_rect or
pending_window_rect if there's a pending move. I've changed this to return the
window_screen_rect_ and added a viewRect method for callers that actually
expect the view. I also did some cleanup and replaced the redundant
RootWindowRect with windowRect.

BUG=638671

Committed: https://crrev.com/6b08cd23a5bbe4e1d95bb7e0431fc743e6b0180c
Cr-Commit-Position: refs/heads/master@{#423030}
==========

[commit-bot: I haz the power, 2016-10-05 00:57:59]

Patchset 9 (id:??) landed as
https://crrev.com/6b08cd23a5bbe4e1d95bb7e0431fc743e6b0180c
Cr-Commit-Position: refs/heads/master@{#423030}