Update nested OOPIF's screen rects when parent is repositioned in page.

Update nested OOPIF's screen rects when parent is repositioned in page.

When an out-of-process iframe is repositioned in its parent frame, a
FrameHostMsg_FrameRectChanged message prompts the browser to notify
the OOPIF with its new screen position. However, it needs to
propagate this message to any further descendant OOPIFs whose positions
are implicitly affected.

This patch adds to FrameTree the ability to iterate through a subtree of
FrameTreeNodes, and uses this to find descendant OOPIFs and update
their screen rects.

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

Committed: https://crrev.com/61b6c251433a39bf4e77b23df172b42df2bd1a2e
Cr-Commit-Position: refs/heads/master@{#382598}

[kenrb, 2016-03-21 20:53:46]

Description was changed from

==========
Update nested OOPIF's screen rects when parent is repositioned in page.

When an out-of-process iframe is repositioned in its parent frame, a
FrameHostMsg_FrameRectChanged message prompts the browser to notify
the OOPIF with its new screen position. However, it needs to
propagate this message to any further descendant OOPIFs whose positions
are implicitly affected.

This patch adds to FrameTree the ability to iterate through a subtree of
FrameTreeNodes, and uses this to find descendant OOPIFs and update
their screen rects.

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

to

==========
Update nested OOPIF's screen rects when parent is repositioned in page.

When an out-of-process iframe is repositioned in its parent frame, a
FrameHostMsg_FrameRectChanged message prompts the browser to notify
the OOPIF with its new screen position. However, it needs to
propagate this message to any further descendant OOPIFs whose positions
are implicitly affected.

This patch adds to FrameTree the ability to iterate through a subtree of
FrameTreeNodes, and uses this to find descendant OOPIFs and update
their screen rects.

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

[kenrb, 2016-03-21 20:59:44]

Description was changed from

==========
Update nested OOPIF's screen rects when parent is repositioned in page.

When an out-of-process iframe is repositioned in its parent frame, a
FrameHostMsg_FrameRectChanged message prompts the browser to notify
the OOPIF with its new screen position. However, it needs to
propagate this message to any further descendant OOPIFs whose positions
are implicitly affected.

This patch adds to FrameTree the ability to iterate through a subtree of
FrameTreeNodes, and uses this to find descendant OOPIFs and update
their screen rects.

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

to

==========
Update nested OOPIF's screen rects when parent is repositioned in page.

When an out-of-process iframe is repositioned in its parent frame, a
FrameHostMsg_FrameRectChanged message prompts the browser to notify
the OOPIF with its new screen position. However, it needs to
propagate this message to any further descendant OOPIFs whose positions
are implicitly affected.

This patch adds to FrameTree the ability to iterate through a subtree of
FrameTreeNodes, and uses this to find descendant OOPIFs and update
their screen rects.

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

[kenrb, 2016-03-21 20:59:44]

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

[kenrb, 2016-03-21 21:00:40]

PTAL:
- lfg I'd appreciate comment on whether this fixes the bug
- nasko for content/ OWNER review

[lfg, 2016-03-22 01:54:42]

Nice, that was a quick fix! lgtm

[nasko, 2016-03-22 13:56:55]

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

https://codereview.chromium.org/1820043002/diff/1/content/browser/frame_host/...
content/browser/frame_host/cross_process_frame_connector.cc:264: if (node !=
proxy_node &&
nit: Why not add a SubtreeNodes() call that takes a "node_to_skip", similar to
existing ones and avoid this check?

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

https://codereview.chromium.org/1820043002/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree.h:151: // Returns a range to iterall over
all FrameTreeNodes in a subtree of the
s/iterall/iterate/? here and below.

https://codereview.chromium.org/1820043002/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree.h:162: ConstNodeRange
ConstSubtreeNodes(FrameTreeNode* subtree_root);
Nick wanted to get rid of the const version. Maybe no need to add one for
subtrees?

https://codereview.chromium.org/1820043002/diff/1/content/browser/site_per_pr...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1820043002/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:5245:
MAYBE_NestedPagePopupMenuTest) {
nit: NestedFrames? MultipleNestedFrames? We've been avoiding Page usage in
general.

https://codereview.chromium.org/1820043002/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:5262: //   \-> b_node
(out-of-process)
nit: "out-of-process from root and c_node"? It will make it clearer also on the
next line that each of them are out of process from each other.

https://codereview.chromium.org/1820043002/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:5271:
navigation_observer.Wait();
No need to wait any longer. I've landed the CL to make it wait for document
load, so this observer can be removed.

https://codereview.chromium.org/1820043002/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:5322: // doesn't seem to be any
better way to find out when this happens.
Can't be done with another message filter?

https://codereview.chromium.org/1820043002/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:5341: popup_rect =
filter->last_initial_rect();
minor nit: if the rect can be updated, why call it "initial"?

[kenrb, 2016-03-22 15:40:20]

Thanks for the review. Any other issues?

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

https://codereview.chromium.org/1820043002/diff/1/content/browser/frame_host/...
content/browser/frame_host/cross_process_frame_connector.cc:264: if (node !=
proxy_node &&
On 2016/03/22 13:56:54, nasko (slow) wrote:
> nit: Why not add a SubtreeNodes() call that takes a "node_to_skip", similar to
> existing ones and avoid this check?

The NodesExcept code in frame_tree.cc excludes an entire subtree, with that node
as the root, which doesn't really work here. It appears to only be used in
testing so far, in any case.

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

https://codereview.chromium.org/1820043002/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree.h:151: // Returns a range to iterall over
all FrameTreeNodes in a subtree of the
On 2016/03/22 13:56:54, nasko (slow) wrote:
> s/iterall/iterate/? here and below.

Done.

https://codereview.chromium.org/1820043002/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree.h:162: ConstNodeRange
ConstSubtreeNodes(FrameTreeNode* subtree_root);
On 2016/03/22 13:56:54, nasko (slow) wrote:
> Nick wanted to get rid of the const version. Maybe no need to add one for
> subtrees?

Done.

https://codereview.chromium.org/1820043002/diff/1/content/browser/site_per_pr...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1820043002/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:5245:
MAYBE_NestedPagePopupMenuTest) {
On 2016/03/22 13:56:54, nasko (slow) wrote:
> nit: NestedFrames? MultipleNestedFrames? We've been avoiding Page usage in
> general.

That's fine. PagePopup is used throughout Blink to refer to these kinds of
popups, but in content PopupMenu is probably better. Done.

https://codereview.chromium.org/1820043002/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:5262: //   \-> b_node
(out-of-process)
On 2016/03/22 13:56:54, nasko (slow) wrote:
> nit: "out-of-process from root and c_node"? It will make it clearer also on
the
> next line that each of them are out of process from each other.

Done.

https://codereview.chromium.org/1820043002/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:5271:
navigation_observer.Wait();
On 2016/03/22 13:56:54, nasko (slow) wrote:
> No need to wait any longer. I've landed the CL to make it wait for document
> load, so this observer can be removed.

Done.

https://codereview.chromium.org/1820043002/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:5322: // doesn't seem to be any
better way to find out when this happens.
On 2016/03/22 13:56:54, nasko (slow) wrote:
> Can't be done with another message filter?

Not easily. We need to wait until after the message has been processed and
passed on, so hooking the incoming message will execute too soon.

https://codereview.chromium.org/1820043002/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:5341: popup_rect =
filter->last_initial_rect();
On 2016/03/22 13:56:54, nasko (slow) wrote:
> minor nit: if the rect can be updated, why call it "initial"?

It's the name of the IPC parameter whose value we are verifying. I felt it made
it clearer to keep with consistent naming.

[nasko, 2016-03-22 16:18:00]

LGTM

https://codereview.chromium.org/1820043002/diff/1/content/browser/site_per_pr...
File content/browser/site_per_process_browsertest.cc (right):

https://codereview.chromium.org/1820043002/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:5322: // doesn't seem to be any
better way to find out when this happens.
On 2016/03/22 15:40:20, kenrb wrote:
> On 2016/03/22 13:56:54, nasko (slow) wrote:
> > Can't be done with another message filter?
> 
> Not easily. We need to wait until after the message has been processed and
> passed on, so hooking the incoming message will execute too soon.

Acknowledged.

https://codereview.chromium.org/1820043002/diff/1/content/browser/site_per_pr...
content/browser/site_per_process_browsertest.cc:5341: popup_rect =
filter->last_initial_rect();
On 2016/03/22 15:40:20, kenrb wrote:
> On 2016/03/22 13:56:54, nasko (slow) wrote:
> > minor nit: if the rect can be updated, why call it "initial"?
> 
> It's the name of the IPC parameter whose value we are verifying. I felt it
made
> it clearer to keep with consistent naming.

Acknowledged.

[kenrb, 2016-03-22 16:18:37]

The CQ bit was checked by kenrb@chromium.org

[kenrb, 2016-03-22 16:18:37]

The patchset sent to the CQ was uploaded after l-g-t-m from lfg@chromium.org
Link to the patchset: https://codereview.chromium.org/1820043002/#ps20001
(title: "nasko comments addressed")

[commit-bot: I haz the power, 2016-03-22 16:18:49]

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

[commit-bot: I haz the power, 2016-03-22 17:37:24]

Description was changed from

==========
Update nested OOPIF's screen rects when parent is repositioned in page.

When an out-of-process iframe is repositioned in its parent frame, a
FrameHostMsg_FrameRectChanged message prompts the browser to notify
the OOPIF with its new screen position. However, it needs to
propagate this message to any further descendant OOPIFs whose positions
are implicitly affected.

This patch adds to FrameTree the ability to iterate through a subtree of
FrameTreeNodes, and uses this to find descendant OOPIFs and update
their screen rects.

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

to

==========
Update nested OOPIF's screen rects when parent is repositioned in page.

When an out-of-process iframe is repositioned in its parent frame, a
FrameHostMsg_FrameRectChanged message prompts the browser to notify
the OOPIF with its new screen position. However, it needs to
propagate this message to any further descendant OOPIFs whose positions
are implicitly affected.

This patch adds to FrameTree the ability to iterate through a subtree of
FrameTreeNodes, and uses this to find descendant OOPIFs and update
their screen rects.

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

[commit-bot: I haz the power, 2016-03-22 17:37:25]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-03-22 17:39:10]

Description was changed from

==========
Update nested OOPIF's screen rects when parent is repositioned in page.

When an out-of-process iframe is repositioned in its parent frame, a
FrameHostMsg_FrameRectChanged message prompts the browser to notify
the OOPIF with its new screen position. However, it needs to
propagate this message to any further descendant OOPIFs whose positions
are implicitly affected.

This patch adds to FrameTree the ability to iterate through a subtree of
FrameTreeNodes, and uses this to find descendant OOPIFs and update
their screen rects.

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

to

==========
Update nested OOPIF's screen rects when parent is repositioned in page.

When an out-of-process iframe is repositioned in its parent frame, a
FrameHostMsg_FrameRectChanged message prompts the browser to notify
the OOPIF with its new screen position. However, it needs to
propagate this message to any further descendant OOPIFs whose positions
are implicitly affected.

This patch adds to FrameTree the ability to iterate through a subtree of
FrameTreeNodes, and uses this to find descendant OOPIFs and update
their screen rects.

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

Committed: https://crrev.com/61b6c251433a39bf4e77b23df172b42df2bd1a2e
Cr-Commit-Position: refs/heads/master@{#382598}
==========

[commit-bot: I haz the power, 2016-03-22 17:39:11]

Patchset 2 (id:??) landed as
https://crrev.com/61b6c251433a39bf4e77b23df172b42df2bd1a2e
Cr-Commit-Position: refs/heads/master@{#382598}