Make page popups work under OOPIF

Make page popups work under OOPIF

The following changes were needed for select menu popups to work from
an out of process iframe:
- the ShowWidget message has to be processed by the inactive subframe
  RenderViewHostImpl
- the view_screen_rect on the OOPIF RenderWidget must get updated
  properly, reflecting the location and size of the OOPIF on the screen
- ChromeClient::viewportToScreen in Blink has to become frame specific,
  since it needs to use the Widget associated with the local roo

BUG=554119
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/c52e1b4e0b1d9692a192f2a2d2594919849bc510
Cr-Commit-Position: refs/heads/master@{#382209}

[kenrb, 2016-03-17 15:14:32]

Description was changed from

==========
Make page popups work under OOPIF

The following changes were needed for select menu popups to work from
an out of process iframe:
- the ShowWidget message has to be processed by the inactive subframe
  RenderViewHostImpl
- the view_screen_rect on the OOPIF RenderWidget must get updated
  properly, reflecting the location and size of the OOPIF on the screen
- ChromeClient::viewportToScreen in Blink has to become frame specific,
  since it needs to use the Widget associated with the local roo

BUG=554119
==========

to

==========
Make page popups work under OOPIF

The following changes were needed for select menu popups to work from
an out of process iframe:
- the ShowWidget message has to be processed by the inactive subframe
  RenderViewHostImpl
- the view_screen_rect on the OOPIF RenderWidget must get updated
  properly, reflecting the location and size of the OOPIF on the screen
- ChromeClient::viewportToScreen in Blink has to become frame specific,
  since it needs to use the Widget associated with the local roo

BUG=554119
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[kenrb, 2016-03-17 15:15:20]

Description was changed from

==========
Make page popups work under OOPIF

The following changes were needed for select menu popups to work from
an out of process iframe:
- the ShowWidget message has to be processed by the inactive subframe
  RenderViewHostImpl
- the view_screen_rect on the OOPIF RenderWidget must get updated
  properly, reflecting the location and size of the OOPIF on the screen
- ChromeClient::viewportToScreen in Blink has to become frame specific,
  since it needs to use the Widget associated with the local roo

BUG=554119
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Make page popups work under OOPIF

The following changes were needed for select menu popups to work from
an out of process iframe:
- the ShowWidget message has to be processed by the inactive subframe
  RenderViewHostImpl
- the view_screen_rect on the OOPIF RenderWidget must get updated
  properly, reflecting the location and size of the OOPIF on the screen
- ChromeClient::viewportToScreen in Blink has to become frame specific,
  since it needs to use the Widget associated with the local roo

BUG=554119
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[kenrb, 2016-03-17 15:15:20]

kenrb@chromium.org changed reviewers:
+ dcheng@chromium.org, lfg@chromium.org, nasko@chromium.org

[kenrb, 2016-03-17 15:16:27]

I haven't finished writing tests for this yet, but I would appreciate any
comments on the code in the meantime.

PTAL:
- lfg for the view_screen_rect propagation
- nasko for content/
- dcheng for blink

[nasko, 2016-03-17 17:25:45]

Some initial comments on content/ bits.

https://codereview.chromium.org/1814863002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/1814863002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_impl.cc:986: ->is_active())
Is this the correct check to make here? I think this will always evaluate to
true. Can you put a comment with what the intent is here?

https://codereview.chromium.org/1814863002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1814863002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:2231: // RenderWidgetHost,
this can be used to update all RenderWidgetHosts.
RenderViewHost is no longer RenderWidgetHost, right? Why not remove the above
call and just iterate through all the nodes?
Also, even if they did inherit, why does that matter?

https://codereview.chromium.org/1814863002/diff/1/content/common/swapped_out_...
File content/common/swapped_out_messages.cc (right):

https://codereview.chromium.org/1814863002/diff/1/content/common/swapped_out_...
content/common/swapped_out_messages.cc:53: case ViewHostMsg_CreateWidget::ID:
Generic question: why do we need to route this through RV(H) instead of RF(H) or
even RW(H)? It seems to me that we must know which frame is causing the popup
menu to appear, so it is a bit strange to see it routed through the RenderView.

[kenrb, 2016-03-17 18:59:48]

https://codereview.chromium.org/1814863002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/1814863002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_impl.cc:986: ->is_active())
On 2016/03/17 17:25:45, nasko (slow) wrote:
> Is this the correct check to make here? I think this will always evaluate to
> true. Can you put a comment with what the intent is here?

If the is_active_ bit is never false on a top-level RenderViewHost, then we can
remove this check. I wasn't sure about this, but it looks like the condition
might become redundant with the death of swapped out state. Can I safely remove
this now?

https://codereview.chromium.org/1814863002/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1814863002/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:2231: // RenderWidgetHost,
this can be used to update all RenderWidgetHosts.
On 2016/03/17 17:25:45, nasko (slow) wrote:
> RenderViewHost is no longer RenderWidgetHost, right? Why not remove the above
> call and just iterate through all the nodes?
> Also, even if they did inherit, why does that matter?

Thanks for the tip, I didn't realize the top-level RenderFrameHost's widget
pointer points to the main RenderWidgetHost now. Fixed.

https://codereview.chromium.org/1814863002/diff/1/content/common/swapped_out_...
File content/common/swapped_out_messages.cc (right):

https://codereview.chromium.org/1814863002/diff/1/content/common/swapped_out_...
content/common/swapped_out_messages.cc:53: case ViewHostMsg_CreateWidget::ID:
On 2016/03/17 17:25:45, nasko (slow) wrote:
> Generic question: why do we need to route this through RV(H) instead of RF(H)
or
> even RW(H)? It seems to me that we must know which frame is causing the popup
> menu to appear, so it is a bit strange to see it routed through the
RenderView.

I had initially written a fairly large patch that moved all of this from
RenderView to RenderFrame, but then realized it didn't really solve the problem
and was a lot of unnecessary churn. Since the widget ends up getting owned by
the WebContents anyway, keeping this as page-level state seems to make sense.
The linkage between the popup and the owning element is maintained in the
renderer, so it just becomes extra complexity to try to associate it with a
local root.

[nasko, 2016-03-17 19:10:46]

https://codereview.chromium.org/1814863002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/1814863002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_impl.cc:986: ->is_active())
On 2016/03/17 18:59:48, kenrb wrote:
> On 2016/03/17 17:25:45, nasko (slow) wrote:
> > Is this the correct check to make here? I think this will always evaluate to
> > true. Can you put a comment with what the intent is here?
> 
> If the is_active_ bit is never false on a top-level RenderViewHost, then we
can
> remove this check. I wasn't sure about this, but it looks like the condition
> might become redundant with the death of swapped out state. Can I safely
remove
> this now?

Oops, I've read it current_frame_host. I still think the comment applies though,
as the implementation of current_host() takes the current RFH and grabs its
RenderViewHost. It is considered active when the RFH is the current one. Once we
start on the path of swapping it out and the new RFH has been committed, the RVH
is considered inactive, but after commit we will get the new RFH's RVH.

I'm curious though what will be the effect of dropping this check? I think the
check was enforcing that this IPC is not received from a swapped out RVH, which
you just relaxed as a requirement. Is there any attack possible if a compromised
renderer for one of the oopifs ends up sending this IPC to the browser?

https://codereview.chromium.org/1814863002/diff/1/content/common/swapped_out_...
File content/common/swapped_out_messages.cc (right):

https://codereview.chromium.org/1814863002/diff/1/content/common/swapped_out_...
content/common/swapped_out_messages.cc:53: case ViewHostMsg_CreateWidget::ID:
On 2016/03/17 18:59:48, kenrb wrote:
> On 2016/03/17 17:25:45, nasko (slow) wrote:
> > Generic question: why do we need to route this through RV(H) instead of
RF(H)
> or
> > even RW(H)? It seems to me that we must know which frame is causing the
popup
> > menu to appear, so it is a bit strange to see it routed through the
> RenderView.
> 
> I had initially written a fairly large patch that moved all of this from
> RenderView to RenderFrame, but then realized it didn't really solve the
problem
> and was a lot of unnecessary churn. Since the widget ends up getting owned by
> the WebContents anyway, keeping this as page-level state seems to make sense.
> The linkage between the popup and the owning element is maintained in the
> renderer, so it just becomes extra complexity to try to associate it with a
> local root.

Does it make sense to make it PageMsg in a follow up CL then? We can route it
through the top-level frame or proxy, which will get it delivered to WebContents
without the need to have swapped out exceptions.

[kenrb, 2016-03-17 20:00:06]

https://codereview.chromium.org/1814863002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_view_host_impl.cc (right):

https://codereview.chromium.org/1814863002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_view_host_impl.cc:986: ->is_active())
On 2016/03/17 19:10:46, nasko (slow) wrote:
> On 2016/03/17 18:59:48, kenrb wrote:
> > On 2016/03/17 17:25:45, nasko (slow) wrote:
> > > Is this the correct check to make here? I think this will always evaluate
to
> > > true. Can you put a comment with what the intent is here?
> > 
> > If the is_active_ bit is never false on a top-level RenderViewHost, then we
> can
> > remove this check. I wasn't sure about this, but it looks like the condition
> > might become redundant with the death of swapped out state. Can I safely
> remove
> > this now?
> 
> Oops, I've read it current_frame_host. I still think the comment applies
though,
> as the implementation of current_host() takes the current RFH and grabs its
> RenderViewHost. It is considered active when the RFH is the current one. Once
we
> start on the path of swapping it out and the new RFH has been committed, the
RVH
> is considered inactive, but after commit we will get the new RFH's RVH.
> 
> I'm curious though what will be the effect of dropping this check? I think the
> check was enforcing that this IPC is not received from a swapped out RVH,
which
> you just relaxed as a requirement. Is there any attack possible if a
compromised
> renderer for one of the oopifs ends up sending this IPC to the browser?

Possibly it could be used for some kind of UI redressing attack, but we consider
those out of scope.

It sounds like this check can go, in any case.

https://codereview.chromium.org/1814863002/diff/1/content/common/swapped_out_...
File content/common/swapped_out_messages.cc (right):

https://codereview.chromium.org/1814863002/diff/1/content/common/swapped_out_...
content/common/swapped_out_messages.cc:53: case ViewHostMsg_CreateWidget::ID:
On 2016/03/17 19:10:46, nasko (slow) wrote:
> On 2016/03/17 18:59:48, kenrb wrote:
> > On 2016/03/17 17:25:45, nasko (slow) wrote:
> > > Generic question: why do we need to route this through RV(H) instead of
> RF(H)
> > or
> > > even RW(H)? It seems to me that we must know which frame is causing the
> popup
> > > menu to appear, so it is a bit strange to see it routed through the
> > RenderView.
> > 
> > I had initially written a fairly large patch that moved all of this from
> > RenderView to RenderFrame, but then realized it didn't really solve the
> problem
> > and was a lot of unnecessary churn. Since the widget ends up getting owned
by
> > the WebContents anyway, keeping this as page-level state seems to make
sense.
> > The linkage between the popup and the owning element is maintained in the
> > renderer, so it just becomes extra complexity to try to associate it with a
> > local root.
> 
> Does it make sense to make it PageMsg in a follow up CL then? We can route it
> through the top-level frame or proxy, which will get it delivered to
WebContents
> without the need to have swapped out exceptions.

I have been assuming that every ViewMsg will get turned into a PageMsg, FrameMsg
or WidgetMsg at some point in the future. Is there a reason that kind of cleanup
would be a priority?

[dcheng, 2016-03-17 20:21:34]

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

[dcheng, 2016-03-17 20:21:35]

+bokan to look at the blink bits

(I'll think more about the plumbing questions tonight, will be offline for a
couple hours)

https://codereview.chromium.org/1814863002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/cross_process_frame_connector.h (right):

https://codereview.chromium.org/1814863002/diff/1/content/browser/frame_host/...
content/browser/frame_host/cross_process_frame_connector.h:132: void
SetRect(gfx::Rect frame_rect);
Pass this by const ref.

[nasko, 2016-03-17 21:05:32]

https://codereview.chromium.org/1814863002/diff/1/content/common/swapped_out_...
File content/common/swapped_out_messages.cc (right):

https://codereview.chromium.org/1814863002/diff/1/content/common/swapped_out_...
content/common/swapped_out_messages.cc:53: case ViewHostMsg_CreateWidget::ID:
On 2016/03/17 20:00:06, kenrb wrote:
> On 2016/03/17 19:10:46, nasko (slow) wrote:
> > On 2016/03/17 18:59:48, kenrb wrote:
> > > On 2016/03/17 17:25:45, nasko (slow) wrote:
> > > > Generic question: why do we need to route this through RV(H) instead of
> > RF(H)
> > > or
> > > > even RW(H)? It seems to me that we must know which frame is causing the
> > popup
> > > > menu to appear, so it is a bit strange to see it routed through the
> > > RenderView.
> > > 
> > > I had initially written a fairly large patch that moved all of this from
> > > RenderView to RenderFrame, but then realized it didn't really solve the
> > problem
> > > and was a lot of unnecessary churn. Since the widget ends up getting owned
> by
> > > the WebContents anyway, keeping this as page-level state seems to make
> sense.
> > > The linkage between the popup and the owning element is maintained in the
> > > renderer, so it just becomes extra complexity to try to associate it with
a
> > > local root.
> > 
> > Does it make sense to make it PageMsg in a follow up CL then? We can route
it
> > through the top-level frame or proxy, which will get it delivered to
> WebContents
> > without the need to have swapped out exceptions.
> 
> I have been assuming that every ViewMsg will get turned into a PageMsg,
FrameMsg
> or WidgetMsg at some point in the future. Is there a reason that kind of
cleanup
> would be a priority?

I didn't mean it should be a priority. It was a curiosity question. Yes, all
ViewMsg will disappear eventually. I was asking which of the three types of
message will it map to.

[bokan, 2016-03-18 15:27:36]

https://codereview.chromium.org/1814863002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/1814863002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:510: IntRect
ChromeClientImpl::viewportToScreen(const IntRect& rectInViewport, const Widget*
widget) const
It seems like an awkward thing to have to pass in a FrameView to convert from an
unrelated viewport rect to an unrelated screen rect. Is there any way
ChromeClientImpl can figure out the local frame root of the current process on
its own?

https://codereview.chromium.org/1814863002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:514:
ASSERT(widget->isFrameView());
Just make the parameter a FrameView*?

https://codereview.chromium.org/1814863002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:518: if
(WebLocalFrameImpl::fromFrame(frame) &&
WebLocalFrameImpl::fromFrame(frame)->frameWidget()) {
Seems like the only difference in these branches is who's client you're using.
You could make this more clear by conditionally assigning into a
WebWidgetClient* and then the rest of the code can be outside the conditional
block.

[lfg, 2016-03-18 15:46:00]

https://codereview.chromium.org/1814863002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/1814863002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:75: }
I don't think this is enough, we need to send the screen rects to all the
descendant local roots in the frame tree.

[kenrb, 2016-03-18 17:46:10]

Thanks for the reviews. PTAL again?

https://codereview.chromium.org/1814863002/diff/1/content/browser/frame_host/...
File content/browser/frame_host/cross_process_frame_connector.h (right):

https://codereview.chromium.org/1814863002/diff/1/content/browser/frame_host/...
content/browser/frame_host/cross_process_frame_connector.h:132: void
SetRect(gfx::Rect frame_rect);
On 2016/03/17 20:21:35, dcheng wrote:
> Pass this by const ref.

Done.

https://codereview.chromium.org/1814863002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/1814863002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:75: }
On 2016/03/18 15:46:00, lfg wrote:
> I don't think this is enough, we need to send the screen rects to all the
> descendant local roots in the frame tree.

I don't think I understand this. This should be called independently for each
local root, since each CrossProcessFrameConnector should get an IPC with the
rect in it.

https://codereview.chromium.org/1814863002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/1814863002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:510: IntRect
ChromeClientImpl::viewportToScreen(const IntRect& rectInViewport, const Widget*
widget) const
On 2016/03/18 15:27:36, bokan wrote:
> It seems like an awkward thing to have to pass in a FrameView to convert from
an
> unrelated viewport rect to an unrelated screen rect. Is there any way
> ChromeClientImpl can figure out the local frame root of the current process on
> its own?

The trouble is that there can be many local frame roots in the current process,
such as if a page creates 2 out-of-process iframes that have the same origin as
each other.

https://codereview.chromium.org/1814863002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:514:
ASSERT(widget->isFrameView());
On 2016/03/18 15:27:36, bokan wrote:
> Just make the parameter a FrameView*?

I think it's a layering violation for HostWindow.h (in platform) to include a
file from core.

https://codereview.chromium.org/1814863002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:518: if
(WebLocalFrameImpl::fromFrame(frame) &&
WebLocalFrameImpl::fromFrame(frame)->frameWidget()) {
On 2016/03/18 15:27:36, bokan wrote:
> Seems like the only difference in these branches is who's client you're using.
> You could make this more clear by conditionally assigning into a
> WebWidgetClient* and then the rest of the code can be outside the conditional
> block.

Done.

[lfg, 2016-03-18 18:53:54]

https://codereview.chromium.org/1814863002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/1814863002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:75: }
On 2016/03/18 17:46:10, kenrb wrote:
> On 2016/03/18 15:46:00, lfg wrote:
> > I don't think this is enough, we need to send the screen rects to all the
> > descendant local roots in the frame tree.
> 
> I don't think I understand this. This should be called independently for each
> local root, since each CrossProcessFrameConnector should get an IPC with the
> rect in it.

Let's say you have a FrameTree like:

A
 |
B
 |
C

If in the renderer for A the position of B is changed, we should get an IPC for
the change in the frame rect. However, the location of C also changed, but I
don't think renderer B will send an IPC for the location change.

[bokan, 2016-03-18 19:25:43]

Blink changes lgtm

https://codereview.chromium.org/1814863002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/1814863002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:510: IntRect
ChromeClientImpl::viewportToScreen(const IntRect& rectInViewport, const Widget*
widget) const
On 2016/03/18 17:46:10, kenrb wrote:
> On 2016/03/18 15:27:36, bokan wrote:
> > It seems like an awkward thing to have to pass in a FrameView to convert
from
> an
> > unrelated viewport rect to an unrelated screen rect. Is there any way
> > ChromeClientImpl can figure out the local frame root of the current process
on
> > its own?
> 
> The trouble is that there can be many local frame roots in the current
process,
> such as if a page creates 2 out-of-process iframes that have the same origin
as
> each other.

Ah, I see. ChromeClient still seems like an odd place for this function in the
OOPIF world. Perhaps FrameHost would be a better place for it? Anyway, I suppose
it doesn't matter much right now and you have a better intuition for where it
might/might not belong so I'm fine with this.

https://codereview.chromium.org/1814863002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:514:
ASSERT(widget->isFrameView());
On 2016/03/18 17:46:10, kenrb wrote:
> On 2016/03/18 15:27:36, bokan wrote:
> > Just make the parameter a FrameView*?
> 
> I think it's a layering violation for HostWindow.h (in platform) to include a
> file from core.

Ah, yah, you're right. Though that seems to imply that Widget is too broad but
changing that would be a  larger refactoring...

https://codereview.chromium.org/1814863002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/1814863002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:559: } else {
Nit: no braces on single line if/else

[kenrb, 2016-03-18 20:03:18]

kenrb@chromium.org changed reviewers:
+ jbroman@chromium.org

[kenrb, 2016-03-18 20:03:19]

nasko: PTAL again? Lucas pointed out a bug, but if the impact is very small and
if we want to move the experiment to dev channel next week it is probably better
to follow up with a fix on Monday.

jbroman: PTAL HostWindow.h?

https://codereview.chromium.org/1814863002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/1814863002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:75: }
On 2016/03/18 18:53:53, lfg wrote:
> On 2016/03/18 17:46:10, kenrb wrote:
> > On 2016/03/18 15:46:00, lfg wrote:
> > > I don't think this is enough, we need to send the screen rects to all the
> > > descendant local roots in the frame tree.
> > 
> > I don't think I understand this. This should be called independently for
each
> > local root, since each CrossProcessFrameConnector should get an IPC with the
> > rect in it.
> 
> Let's say you have a FrameTree like:
> 
> A
>  |
> B
>  |
> C
> 
> If in the renderer for A the position of B is changed, we should get an IPC
for
> the change in the frame rect. However, the location of C also changed, but I
> don't think renderer B will send an IPC for the location change.

This is a really good point, thanks for catching it. Given time constraints, I
am going to propose that (if this is the only issue), this CL goes in as is and
I will file a bug to fix next week.

https://codereview.chromium.org/1814863002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/web/ChromeClientImpl.cpp (right):

https://codereview.chromium.org/1814863002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/web/ChromeClientImpl.cpp:559: } else {
On 2016/03/18 19:25:42, bokan wrote:
> Nit: no braces on single line if/else

Done.

[lfg, 2016-03-18 20:06:20]

https://codereview.chromium.org/1814863002/diff/20001/content/browser/frame_h...
File content/browser/frame_host/render_widget_host_view_child_frame.cc (right):

https://codereview.chromium.org/1814863002/diff/20001/content/browser/frame_h...
content/browser/frame_host/render_widget_host_view_child_frame.cc:75: }
On 2016/03/18 20:03:19, kenrb wrote:
> On 2016/03/18 18:53:53, lfg wrote:
> > On 2016/03/18 17:46:10, kenrb wrote:
> > > On 2016/03/18 15:46:00, lfg wrote:
> > > > I don't think this is enough, we need to send the screen rects to all
the
> > > > descendant local roots in the frame tree.
> > > 
> > > I don't think I understand this. This should be called independently for
> each
> > > local root, since each CrossProcessFrameConnector should get an IPC with
the
> > > rect in it.
> > 
> > Let's say you have a FrameTree like:
> > 
> > A
> >  |
> > B
> >  |
> > C
> > 
> > If in the renderer for A the position of B is changed, we should get an IPC
> for
> > the change in the frame rect. However, the location of C also changed, but I
> > don't think renderer B will send an IPC for the location change.
> 
> This is a really good point, thanks for catching it. Given time constraints, I
> am going to propose that (if this is the only issue), this CL goes in as is
and
> I will file a bug to fix next week.

Sounds good. lgtm.

[jbroman, 2016-03-18 20:17:04]

Source/platform/ lgtm

[nasko, 2016-03-18 22:18:20]

Thanks for all the fixes! LGTM

[nasko, 2016-03-18 22:34:17]

The CQ bit was checked by nasko@chromium.org

[nasko, 2016-03-18 22:34:18]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
jbroman@chromium.org, lfg@chromium.org
Link to the patchset: https://codereview.chromium.org/1814863002/#ps160001
(title: "Added TestNavigationObserver to test to fix flakiness")

[commit-bot: I haz the power, 2016-03-18 22:34:56]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1814863002/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1814863002/160001

[commit-bot: I haz the power, 2016-03-18 23:54:50]

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

[commit-bot: I haz the power, 2016-03-18 23:54:51]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[kenrb, 2016-03-19 18:40:04]

The CQ bit was checked by kenrb@chromium.org

[kenrb, 2016-03-19 18:40:04]

The patchset sent to the CQ was uploaded after l-g-t-m from bokan@chromium.org,
jbroman@chromium.org, nasko@chromium.org, lfg@chromium.org
Link to the patchset: https://codereview.chromium.org/1814863002/#ps180001
(title: "Disabled test on Mac")

[commit-bot: I haz the power, 2016-03-19 18:40:13]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1814863002/180001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1814863002/180001

[commit-bot: I haz the power, 2016-03-20 01:03:45]

Description was changed from

==========
Make page popups work under OOPIF

The following changes were needed for select menu popups to work from
an out of process iframe:
- the ShowWidget message has to be processed by the inactive subframe
  RenderViewHostImpl
- the view_screen_rect on the OOPIF RenderWidget must get updated
  properly, reflecting the location and size of the OOPIF on the screen
- ChromeClient::viewportToScreen in Blink has to become frame specific,
  since it needs to use the Widget associated with the local roo

BUG=554119
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Make page popups work under OOPIF

The following changes were needed for select menu popups to work from
an out of process iframe:
- the ShowWidget message has to be processed by the inactive subframe
  RenderViewHostImpl
- the view_screen_rect on the OOPIF RenderWidget must get updated
  properly, reflecting the location and size of the OOPIF on the screen
- ChromeClient::viewportToScreen in Blink has to become frame specific,
  since it needs to use the Widget associated with the local roo

BUG=554119
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-03-20 01:03:46]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-03-20 01:05:05]

Description was changed from

==========
Make page popups work under OOPIF

The following changes were needed for select menu popups to work from
an out of process iframe:
- the ShowWidget message has to be processed by the inactive subframe
  RenderViewHostImpl
- the view_screen_rect on the OOPIF RenderWidget must get updated
  properly, reflecting the location and size of the OOPIF on the screen
- ChromeClient::viewportToScreen in Blink has to become frame specific,
  since it needs to use the Widget associated with the local roo

BUG=554119
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Make page popups work under OOPIF

The following changes were needed for select menu popups to work from
an out of process iframe:
- the ShowWidget message has to be processed by the inactive subframe
  RenderViewHostImpl
- the view_screen_rect on the OOPIF RenderWidget must get updated
  properly, reflecting the location and size of the OOPIF on the screen
- ChromeClient::viewportToScreen in Blink has to become frame specific,
  since it needs to use the Widget associated with the local roo

BUG=554119
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/c52e1b4e0b1d9692a192f2a2d2594919849bc510
Cr-Commit-Position: refs/heads/master@{#382209}
==========

[commit-bot: I haz the power, 2016-03-20 01:05:06]

Patchset 10 (id:??) landed as
https://crrev.com/c52e1b4e0b1d9692a192f2a2d2594919849bc510
Cr-Commit-Position: refs/heads/master@{#382209}