Cross-process iframe accessibility.

Cross-process iframe accessibility.

This change completes the plumbing to join cross-process iframes into a single composed accessibility tree on platforms that implement native accessibility APIs (Windows, Mac, Android).

Further work will be needed to update some accessibility API implementations to be multi-frame-aware.

BUG=368298
Committed: https://crrev.com/387942c041da17ea6337bc0a81e96619e67e4ac4
Cr-Commit-Position: refs/heads/master@{#294118}

Committed: https://crrev.com/0b5d2481a5dc4f3a1ed812b174a4da8dde1b64c2
Cr-Commit-Position: refs/heads/master@{#294210}

[dmazzoni, 2014-05-02 06:24:27]

This change depends on: https://codereview.chromium.org/252253002/

creis: I think this is a good start, but I need help with two things before this
gets a more detailed review:

* I'm violating DEPS by including cross_process_frame_connector.h and
render_widget_host_view_child_frame.h from c/b/renderer_host - could you help me
figure out how to fix this? 

* Could we refactor site_per_process_browsertest so that I could share as much
of the same test infrastructure as possible rather than duplicating it in this
browser test? Any thoughts on what to share and what to copy?

dtseng, aboxhall: fyi

[Charlie Reis, 2014-05-02 15:51:44]

Cool!  Thanks for working on this.

I'm going to ask Nick to help with the review, since he's been looking at how to
make writing these tests easier for others.  He also may be able to help with
the CrossProcessFrameConnector question, but we can loop in kenrb@ if not.  One
possibility may be moving OnAccessibilityEvents from RenderViewHostImpl to
RenderFrameHostImpl.

Note that Nasko has recently landed an update to SitePerProcessBrowserTest so
that StartAtDataURL isn't necessary anymore.  See the CrossSiteIframe test; we
haven't updated the others yet.

(I'm also CC'ing site-isolation-reviews@chromium.org, which we include on our
reviews to help keep the team informed.)

[dmazzoni, 2014-05-02 16:31:20]

On Fri, May 2, 2014 at 8:51 AM, <creis@chromium.org> wrote:

> Cool!  Thanks for working on this.
>
> I'm going to ask Nick to help with the review, since he's been looking at
> how to
> make writing these tests easier for others.  He also may be able to help
> with
> the CrossProcessFrameConnector question, but we can loop in kenrb@ if
> not.  One
> possibility may be moving OnAccessibilityEvents from RenderViewHostImpl to
> RenderFrameHostImpl.
>

Does RenderFrameHostImpl have access to the RenderWidgetHostView? I could
move all of the accessibility code to RenderFrameHostImpl if so.

Note that Nasko has recently landed an update to SitePerProcessBrowserTest
> so
> that StartAtDataURL isn't necessary anymore.  See the CrossSiteIframe
> test; we
> haven't updated the others yet.
>

Yeah, I snapshotted it over a week ago. Do you think other
features/components other than accessibility might want to do a site
isolation browser test? I was thinking it might be nice to define a generic
site isolation test with a clean public interface, making it easy for
anyone to write a test of site isolation as easily as:

{
  ... do pre-initialization ...
  LoadChildFrameInSameProcess();
  RenderViewHostImpl* main_frame = GetMainFrame();
  RenderViewHostImpl* child_frame = GetChildFrame();
  ... make note of some state ...

  LoadChildFrameInRemoteProcess();
  main_frame = GetMainFrame();
  child_frame = GetChildFrame();
  ... assert that everything still works ...
}

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Charlie Reis, 2014-05-02 16:34:50]

On 2014/05/02 16:31:20, dmazzoni wrote:
> On Fri, May 2, 2014 at 8:51 AM, <mailto:creis@chromium.org> wrote:
> 
> > Cool!  Thanks for working on this.
> >
> > I'm going to ask Nick to help with the review, since he's been looking at
> > how to
> > make writing these tests easier for others.  He also may be able to help
> > with
> > the CrossProcessFrameConnector question, but we can loop in kenrb@ if
> > not.  One
> > possibility may be moving OnAccessibilityEvents from RenderViewHostImpl to
> > RenderFrameHostImpl.
> >
> 
> Does RenderFrameHostImpl have access to the RenderWidgetHostView? I could
> move all of the accessibility code to RenderFrameHostImpl if so.

Yes.  RenderFrameHostImpl is a friend class of RenderViewHostImpl for now, until
we move everything related (including the widget) over.  Moving it sounds good
to me.

> 
> Note that Nasko has recently landed an update to SitePerProcessBrowserTest
> > so
> > that StartAtDataURL isn't necessary anymore.  See the CrossSiteIframe
> > test; we
> > haven't updated the others yet.
> >
> 
> Yeah, I snapshotted it over a week ago. Do you think other
> features/components other than accessibility might want to do a site
> isolation browser test? I was thinking it might be nice to define a generic
> site isolation test with a clean public interface, making it easy for
> anyone to write a test of site isolation as easily as:
> 
> {
>   ... do pre-initialization ...
>   LoadChildFrameInSameProcess();
>   RenderViewHostImpl* main_frame = GetMainFrame();
>   RenderViewHostImpl* child_frame = GetChildFrame();
>   ... make note of some state ...
> 
>   LoadChildFrameInRemoteProcess();
>   main_frame = GetMainFrame();
>   child_frame = GetChildFrame();
>   ... assert that everything still works ...
> }
> 

Yes, this sort of setup will become common and it would be great to have some
simple helper functions for it.  I know Nick has been looking into that, so I'll
let him comment on their form.

[ncarter (slow), 2014-05-02 19:10:01]

https://codereview.chromium.org/268543008/diff/1/content/browser/accessibilit...
File content/browser/accessibility/frame_tree_accessibility_browsertest.cc
(right):

https://codereview.chromium.org/268543008/diff/1/content/browser/accessibilit...
content/browser/accessibility/frame_tree_accessibility_browsertest.cc:108: bool
NavigateIframeToURL(Shell* window,
One OKR we have for this quarter is to try to get to a world where a
same-process iframe test and a cross-process iframe tests only have to differ by
a couple lines of code. No doubt you can tell why this is a priority :)

The good news is that just this week, nasko landed
https://codereview.chromium.org/248963007 which has a NavigateFrameToURL method
that is used instead of this NavigateIFrameToURL -- you should just be able to
drop that in right now; it blocks until the load is complete, and I think it
should hopefully also make it so that you don't need the
SitePerProcessWebContentsObserver. Look at what happened to the browsertest in
that file, and I think you can just apply the same transformation here.

Another approach I hope to invest in (but haven't gotten to yet) is an easier,
redirect-based trick so that you can just specify cross-site frames in test
.html by tweaking the URL. That's detailed here:
https://groups.google.com/a/chromium.org/forum/#!topic/site-isolation-dev/VID...

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

https://codereview.chromium.org/268543008/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host_impl.cc:1654: iter !=
param.frame_routing_ids.end();
Nothing happens with param.guest_instance_ids here, though the comment suggests
ambition otherwise.

https://codereview.chromium.org/268543008/diff/1/content/browser/renderer_hos...
content/browser/renderer_host/render_view_host_impl.cc:1693: ax_tree_.reset(new
ui::AXTree(manager->SnapshotAXTreeForTesting()));
What prevents this ForTesting() path from becoming active in production chrome?

https://codereview.chromium.org/268543008/diff/1/content/renderer/accessibili...
File content/renderer/accessibility/renderer_accessibility_complete.cc (right):

https://codereview.chromium.org/268543008/diff/1/content/renderer/accessibili...
content/renderer/accessibility/renderer_accessibility_complete.cc:199:
tree_source_.CollectChildFrameIdMapping(
When the method SendPendingAccessibilityEvents returns, it seems that
tree_source_ will be left with two icky stack pointers into the corpse of
event_msg. Can we either give them a proper burial, or make the interface safer?

One possible solution would be to have the members of tree_source_ be maps (vs
map*'s) and after the maps are filled -- maybe at the end of SerializeChanges --
do a map::swap between the event_msg and the tree_source copies. That should be
as efficient, right?

https://codereview.chromium.org/268543008/diff/1/content/test/accessibility_b...
File content/test/accessibility_browser_test_utils.h (right):

https://codereview.chromium.org/268543008/diff/1/content/test/accessibility_b...
content/test/accessibility_browser_test_utils.h:30: RenderViewHostImpl*
render_view_host_impl = NULL);
For some reason, the style guide says you oughta have 2-4 versions of the ctor
here, instead of using default arguments. Because it wouldn't be C++ be without
odious boilerplate, one supposes?
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Default_Arguments

[dmazzoni, 2014-05-06 06:55:12]

Thanks for the feedback - I will get to the specific comments in this
changelist once I've finished restructuring things a bit.

I started a separate change to migrate all of the accessibility code from
RenderWidgetHostImpl and RenderViewHostImpl to RenderFrameHostImpl, that
seems like a good idea and a prereq to get frame tree accessibility right.
One question - previously we'd set an accessibility mode for the widget,
and that would persist across all frames and page navigations. Where's the
right place to store this mode so that all new RenderFrameHostImpls within
the same page are created with the same mode? I thought of storing it in
WebContentsImpl via RenderFrameHostDelegate, but then InterstitialPageImpl
also implements RenderFrameHostDelegate and I'm not sure what to do there.
Maybe the root FrameTreeNode since that persists?

And a related question - what's the right way to call something on all
RenderFrameHostImpls within a page?

On Fri, May 2, 2014 at 9:34 AM, <creis@chromium.org> wrote:

> Does RenderFrameHostImpl have access to the RenderWidgetHostView? I could
>>
> move all of the accessibility code to RenderFrameHostImpl if so.
>>
>
> Yes.  RenderFrameHostImpl is a friend class of RenderViewHostImpl for now,
> until
> we move everything related (including the widget) over.  Moving it sounds
> good
> to me.


>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nasko, 2014-05-06 13:57:10]

On 2014/05/06 06:55:12, dmazzoni wrote:
> Thanks for the feedback - I will get to the specific comments in this
> changelist once I've finished restructuring things a bit.

Do you mind cc'ing me on that CL? I just want to lurk.

> I started a separate change to migrate all of the accessibility code from
> RenderWidgetHostImpl and RenderViewHostImpl to RenderFrameHostImpl, that
> seems like a good idea and a prereq to get frame tree accessibility right.
> One question - previously we'd set an accessibility mode for the widget,
> and that would persist across all frames and page navigations. Where's the
> right place to store this mode so that all new RenderFrameHostImpls within
> the same page are created with the same mode? I thought of storing it in
> WebContentsImpl via RenderFrameHostDelegate, but then InterstitialPageImpl
> also implements RenderFrameHostDelegate and I'm not sure what to do there.
> Maybe the root FrameTreeNode since that persists?

Anything that must persist across navigations and/or is top-level specific
should go to WebContentsImpl. Do we currently preserve accessibility settings
for interstitial pages? To me it seems logical for a tab to keep its settings as
you navigate, even on interstitials, and WebContents is the abstraction for
content in a tab.

> And a related question - what's the right way to call something on all
> RenderFrameHostImpls within a page?

WebContents::ForEachFrame should help you there.
 
> On Fri, May 2, 2014 at 9:34 AM, <mailto:creis@chromium.org> wrote:
> 
> > Does RenderFrameHostImpl have access to the RenderWidgetHostView? I could
> >>
> > move all of the accessibility code to RenderFrameHostImpl if so.
> >>
> >
> > Yes.  RenderFrameHostImpl is a friend class of RenderViewHostImpl for now,
> > until
> > we move everything related (including the widget) over.  Moving it sounds
> > good
> > to me.
> 
> 
> >
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2014-05-06 14:13:51]

On Tue, May 6, 2014 at 6:57 AM, <nasko@chromium.org> wrote:

> On 2014/05/06 06:55:12, dmazzoni wrote:
>
>> Thanks for the feedback - I will get to the specific comments in this
>> changelist once I've finished restructuring things a bit.
>>
>
> Do you mind cc'ing me on that CL? I just want to lurk.


Sure, I'll upload it as soon as I get it to work. :)


>  I started a separate change to migrate all of the accessibility code from
>> RenderWidgetHostImpl and RenderViewHostImpl to RenderFrameHostImpl, that
>> seems like a good idea and a prereq to get frame tree accessibility right.
>> One question - previously we'd set an accessibility mode for the widget,
>> and that would persist across all frames and page navigations. Where's the
>> right place to store this mode so that all new RenderFrameHostImpls within
>> the same page are created with the same mode? I thought of storing it in
>> WebContentsImpl via RenderFrameHostDelegate, but then InterstitialPageImpl
>> also implements RenderFrameHostDelegate and I'm not sure what to do there.
>> Maybe the root FrameTreeNode since that persists?
>>
>
> Anything that must persist across navigations and/or is top-level specific
> should go to WebContentsImpl. Do we currently preserve accessibility
> settings
> for interstitial pages? To me it seems logical for a tab to keep its
> settings as
> you navigate, even on interstitials, and WebContents is the abstraction for
> content in a tab.


OK, I'll move the state to WebContentsImpl. What I don't understand is the
exact relationship between InterstitialPageImpl and WebContentsImpl, since
InterstitialPageImpl implements RenderFrameHostDelegate. If I use
RenderFrameHostDelegate as the way for a RenderFrameHostImpl to get the
state from WebContentsImpl, then what happens when it's showing an
interstitial page and InterstitialPageImpl implements the delegate? Is it
ok to just forward the request to the WebContentsImpl?

And a related question - what's the right way to call something on all
>> RenderFrameHostImpls within a page?
>>
>
> WebContents::ForEachFrame should help you there.


Perfect, makes sense.

- Dominic

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-05-08 16:30:58]

Hey Dominic,
Sorry for the late reply, this email somehow slipped through the cracks :
(. Comments inline.

On Tue, May 6, 2014 at 7:13 AM, Dominic Mazzoni <dmazzoni@chromium.org>wrote:

> On Tue, May 6, 2014 at 6:57 AM, <nasko@chromium.org> wrote:
>
>> On 2014/05/06 06:55:12, dmazzoni wrote:
>>
>>> Thanks for the feedback - I will get to the specific comments in this
>>> changelist once I've finished restructuring things a bit.
>>>
>>
>> Do you mind cc'ing me on that CL? I just want to lurk.
>
>
> Sure, I'll upload it as soon as I get it to work. :)
>
>
>>  I started a separate change to migrate all of the accessibility code
>>> from
>>> RenderWidgetHostImpl and RenderViewHostImpl to RenderFrameHostImpl, that
>>> seems like a good idea and a prereq to get frame tree accessibility
>>> right.
>>> One question - previously we'd set an accessibility mode for the widget,
>>> and that would persist across all frames and page navigations. Where's
>>> the
>>> right place to store this mode so that all new RenderFrameHostImpls
>>> within
>>> the same page are created with the same mode? I thought of storing it in
>>> WebContentsImpl via RenderFrameHostDelegate, but then
>>> InterstitialPageImpl
>>> also implements RenderFrameHostDelegate and I'm not sure what to do
>>> there.
>>> Maybe the root FrameTreeNode since that persists?
>>>
>>
>> Anything that must persist across navigations and/or is top-level specific
>> should go to WebContentsImpl. Do we currently preserve accessibility
>> settings
>> for interstitial pages? To me it seems logical for a tab to keep its
>> settings as
>> you navigate, even on interstitials, and WebContents is the abstraction
>> forSetIsLoading
>> content in a tab.
>
>
> OK, I'll move the state to WebContentsImpl. What I don't understand is the
> exact relationship between InterstitialPageImpl and WebContentsImpl, since
> InterstitialPageImpl implements RenderFrameHostDelegate. If I use
> RenderFrameHostDelegate as the way for a RenderFrameHostImpl to get the
> state from WebContentsImpl, then what happens when it's showing an
> interstitial page and InterstitialPageImpl implements the delegate? Is it
> ok to just forward the request to the WebContentsImpl?
>

InterstitialPageImpl can't talk directly to WebContentsImpl, it has to go
through the public interface. The interstitial is owned by the
RenderFrameHostManager, but it is attached going through
WebContentsImpl::AttachInterstitialPage. I think it is reasonable for
InterstitialPageImpl to have a method to receive the settings you are
interested in and then return them back from calls to
RenderFrameHostDelegate methods.


> And a related question - what's the right way to call something on all
>>> RenderFrameHostImpls within a page?
>>>
>>
>> WebContents::ForEachFrame should help you there.
>
>
> Perfect, makes sense.
>
> - Dominic
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nasko, 2014-05-08 16:32:54]

Hey Dominic,
Sorry for the late reply, this somehow slipped through the cracks : (.
Comments inline.

On Tue, May 6, 2014 at 7:13 AM, Dominic Mazzoni <dmazzoni@chromium.org>wrote:
>
> Anything that must persist across navigations and/or is top-level specific
>> should go to WebContentsImpl. Do we currently preserve accessibility
>> settings
>> for interstitial pages? To me it seems logical for a tab to keep its
>> settings as
>> you navigate, even on interstitials, and WebContents is the abstraction
>> for
>> content in a tab.
>
>
> OK, I'll move the state to WebContentsImpl. What I don't understand is the
> exact relationship between InterstitialPageImpl and WebContentsImpl, since
> InterstitialPageImpl implements RenderFrameHostDelegate. If I use
> RenderFrameHostDelegate as the way for a RenderFrameHostImpl to get the
> state from WebContentsImpl, then what happens when it's showing an
> interstitial page and InterstitialPageImpl implements the delegate? Is it
> ok to just forward the request to the WebContentsImpl?
>

InterstitialPageImpl can't talk directly to WebContentsImpl, it has to go
through the public interface. The interstitial is owned by the
RenderFrameHostManager, but it is attached going through
WebContentsImpl::AttachInterstitialPage.
I think it is reasonable for InterstitialPageImpl to have a method to
receive the settings you are interested in and then return them back from
calls to RenderFrameHostDelegate methods.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2014-05-08 17:23:41]

On Thu, May 8, 2014 at 9:30 AM, Nasko Oskov <nasko@google.com> wrote:

> InterstitialPageImpl can't talk directly to WebContentsImpl, it has to go
> through the public interface. The interstitial is owned by the
> RenderFrameHostManager, but it is attached going through
> WebContentsImpl::AttachInterstitialPage. I think it is reasonable for
> InterstitialPageImpl to have a method to receive the settings you are
> interested in and then return them back from calls to
> RenderFrameHostDelegate methods.
>

So what happens if someone calls a method on a WebContents (public
interface) telling it to enable accessibility, but that tab is currently
showing an interstitial page - does the WebContentsImpl know that there's
an interstitial page, or does it have to relay a message through
RenderFrameHostManager?

Is there any reason we couldn't have an InterstitialPageDelegate that
WebContentsImpl could implement, providing a direct solution?

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-05-08 17:50:50]

On Thu, May 8, 2014 at 10:23 AM, Dominic Mazzoni <dmazzoni@chromium.org>wrote:

> On Thu, May 8, 2014 at 9:30 AM, Nasko Oskov <nasko@google.com> wrote:
>
>> InterstitialPageImpl can't talk directly to WebContentsImpl, it has to go
>> through the public interface. The interstitial is owned by the
>> RenderFrameHostManager, but it is attached going through
>> WebContentsImpl::AttachInterstitialPage. I think it is reasonable for
>> InterstitialPageImpl to have a method to receive the settings you are
>> interested in and then return them back from calls to
>> RenderFrameHostDelegate methods.
>>
>
> So what happens if someone calls a method on a WebContents (public
> interface) telling it to enable accessibility, but that tab is currently
> showing an interstitial page - does the WebContentsImpl know that there's
> an interstitial page, or does it have to relay a message through
> RenderFrameHostManager?
>

WCI has GetInterstitialPage() method that will give you the page if it is
displaying.

>
> Is there any reason we couldn't have an InterstitialPageDelegate that
> WebContentsImpl could implement, providing a direct solution?
>

They are technically peers, as they both embed the structures needed to
display a page (frame tree, RFH, and such). The relationship there is not
the best I think, but I don't have any cycles to do anything about it.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2014-05-08 19:20:37]

On Thu, May 8, 2014 at 10:50 AM, Nasko Oskov <nasko@google.com> wrote:

> WCI has GetInterstitialPage() method that will give you the page if it is
> displaying.
>
>>
>> Is there any reason we couldn't have an InterstitialPageDelegate that
>> WebContentsImpl could implement, providing a direct solution?
>>
>
> They are technically peers, as they both embed the structures needed to
> display a page (frame tree, RFH, and such). The relationship there is not
> the best I think, but I don't have any cycles to do anything about it.
>

Thanks, I think it all makes sense now.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-05-09 16:46:48]

This may be a stupid question, but is there a way to enumerate all
WebContents?

Previously we had code to enumerate every RWH and toggle the accessibility
mode. If we move that state to the WebContents, we need an equivalent way
to hit all of them.

Are there any RWHs we'd miss if we enumerated all WCs and set the
accessibility mode on each frame for each one?

None of this really mattered before when for the most part there was one
RWH per WC.
 On May 8, 2014 12:20 PM, "Dominic Mazzoni" <dmazzoni@chromium.org> wrote:

> On Thu, May 8, 2014 at 10:50 AM, Nasko Oskov <nasko@google.com> wrote:
>
>> WCI has GetInterstitialPage() method that will give you the page if it is
>> displaying.
>>
>>>
>>> Is there any reason we couldn't have an InterstitialPageDelegate that
>>> WebContentsImpl could implement, providing a direct solution?
>>>
>>
>> They are technically peers, as they both embed the structures needed to
>> display a page (frame tree, RFH, and such). The relationship there is not
>> the best I think, but I don't have any cycles to do anything about it.
>>
>
> Thanks, I think it all makes sense now.
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-05-09 16:54:52]

On Fri, May 9, 2014 at 9:46 AM, Dominic Mazzoni <dmazzoni@google.com> wrote:

> This may be a stupid question, but is there a way to enumerate all
> WebContents?
>
There are no stupid questions. I don't know of any such way in the content/
layer.

> Previously we had code to enumerate every RWH and toggle the accessibility
> mode. If we move that state to the WebContents, we need an equivalent way
> to hit all of them.
>
> Are there any RWHs we'd miss if we enumerated all WCs and set the
> accessibility mode on each frame for each one?
>
> None of this really mattered before when for the most part there was one
> RWH per WC.
>
I'd like to hear if John or Charlie have other ideas or whether it makes
sense to have a way to iterate all WebContents.

Without changes to WC, you can still use your current iteration through
RWHs and use WebContents::FromRenderViewHost to get to its WebContents
(after checking that the RWH is RVH). It might meant that you call multiple
times on the same WC, but this can be easily avoided by creating a set of
WC while iterating RWHs and using the set to toggle accessibility.

>  On May 8, 2014 12:20 PM, "Dominic Mazzoni" <dmazzoni@chromium.org> wrote:
>
>> On Thu, May 8, 2014 at 10:50 AM, Nasko Oskov <nasko@google.com> wrote:
>>
>>> WCI has GetInterstitialPage() method that will give you the page if it
>>> is displaying.
>>>
>>>>
>>>> Is there any reason we couldn't have an InterstitialPageDelegate that
>>>> WebContentsImpl could implement, providing a direct solution?
>>>>
>>>
>>> They are technically peers, as they both embed the structures needed to
>>> display a page (frame tree, RFH, and such). The relationship there is not
>>> the best I think, but I don't have any cycles to do anything about it.
>>>
>>
>> Thanks, I think it all makes sense now.
>>
>>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-05-09 18:20:23]

On Fri, May 9, 2014 at 9:54 AM, Nasko Oskov <nasko@google.com> wrote:

> Without changes to WC, you can still use your current iteration through
> RWHs and use WebContents::FromRenderViewHost to get to its WebContents
> (after checking that the RWH is RVH). It might meant that you call multiple
> times on the same WC, but this can be easily avoided by creating a set of
> WC while iterating RWHs and using the set to toggle accessibility.
>

Yes, I thought about doing this but it made me think I might be doing
something wrong.

If there aren't any objections, I think I'd prefer to just write a helper
function to get all WebContents. I took a quick glance through the code to
see other uses of GetRenderWidgetHosts(), and I think there might be some
other cases where it might be preferable to iterate over WebContents
instead.

KeyboardController::NotifyKeyboardBoundsChanging seems to assume that
RWH->GetView()->GetNativeView will always return something, but that
wouldn't be true of a RWHVChildFrame, right?

GuestInformation::GetAll seems to assume there's one WebContents per RWH,
it looks like it could end up running its callback on the same WC more than
once.

- Dominic

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Charlie Reis, 2014-05-09 19:10:26]

On Fri, May 9, 2014 at 11:20 AM, Dominic Mazzoni <dmazzoni@google.com>wrote:

> On Fri, May 9, 2014 at 9:54 AM, Nasko Oskov <nasko@google.com> wrote:
>
>> Without changes to WC, you can still use your current iteration through
>> RWHs and use WebContents::FromRenderViewHost to get to its WebContents
>> (after checking that the RWH is RVH). It might meant that you call multiple
>> times on the same WC, but this can be easily avoided by creating a set of
>> WC while iterating RWHs and using the set to toggle accessibility.
>>
>
> Yes, I thought about doing this but it made me think I might be doing
> something wrong.
>
> If there aren't any objections, I think I'd prefer to just write a helper
> function to get all WebContents. I took a quick glance through the code to
> see other uses of GetRenderWidgetHosts(), and I think there might be some
> other cases where it might be preferable to iterate over WebContents
> instead.
>

Seems reasonable to me, though we should get John's thoughts/approval.


>
> KeyboardController::NotifyKeyboardBoundsChanging seems to assume that
> RWH->GetView()->GetNativeView will always return something, but that
> wouldn't be true of a RWHVChildFrame, right?
>

> GuestInformation::GetAll seems to assume there's one WebContents per RWH,
> it looks like it could end up running its callback on the same WC more than
> once.
>

Yeah, that sounds problematic.  There used to be problems with
GetRenderWidgetHosts() callers including swapped out widgets as well.

Charlie


>
> - Dominic
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jam, 2014-05-09 22:15:18]

On Fri, May 9, 2014 at 12:10 PM, Charlie Reis <creis@chromium.org> wrote:

>
> On Fri, May 9, 2014 at 11:20 AM, Dominic Mazzoni <dmazzoni@google.com>wrote:
>
>> On Fri, May 9, 2014 at 9:54 AM, Nasko Oskov <nasko@google.com> wrote:
>>
>>> Without changes to WC, you can still use your current iteration through
>>> RWHs and use WebContents::FromRenderViewHost to get to its WebContents
>>> (after checking that the RWH is RVH). It might meant that you call multiple
>>> times on the same WC, but this can be easily avoided by creating a set of
>>> WC while iterating RWHs and using the set to toggle accessibility.
>>>
>>
>> Yes, I thought about doing this but it made me think I might be doing
>> something wrong.
>>
>> If there aren't any objections, I think I'd prefer to just write a helper
>> function to get all WebContents. I took a quick glance through the code to
>> see other uses of GetRenderWidgetHosts(), and I think there might be some
>> other cases where it might be preferable to iterate over WebContents
>> instead.
>>
>
> Seems reasonable to me, though we should get John's thoughts/approval.
>

The problem with having a method of getting all WebContents, or getting
notified when they're created, is that it leads to layering violations.
i.e. code in src\apps starts watching for WebContents that are created from
src\chrome. After seeing some of these layering violations, we removed the
global hooks to get notified when WCs are created (code was using it to
maintain a list).

If the accessibility code wants to enable a flag for all WC, where is this
code, in chrome or content? If in chrome, can it iterate across all the
tabs in all the browser objects?

GetRenderWidgetHosts allows layering violations for the same reason, so it
would be great to remove that.

>
>
>>
>> KeyboardController::NotifyKeyboardBoundsChanging seems to assume that
>> RWH->GetView()->GetNativeView will always return something, but that
>> wouldn't be true of a RWHVChildFrame, right?
>>
>
>> GuestInformation::GetAll seems to assume there's one WebContents per RWH,
>> it looks like it could end up running its callback on the same WC more than
>> once.
>>
>
> Yeah, that sounds problematic.  There used to be problems with
> GetRenderWidgetHosts() callers including swapped out widgets as well.
>
> Charlie
>
>
>>
>> - Dominic
>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-05-09 22:22:52]

On Fri, May 9, 2014 at 3:15 PM, John Abd-El-Malek <jam@chromium.org> wrote:

> If the accessibility code wants to enable a flag for all WC, where is this
> code, in chrome or content? If in chrome, can it iterate across all the
> tabs in all the browser objects?
>

There are two places we need this - in AccessibilityUI and
BrowserAccessibilityStateImpl, both times the implementation is inside
content - so would it be okay to have a way to enumerate all
WebContentsImpls instead?

In both cases, I think it makes more sense for both of them to control the
accessibility state of a whole tab (or other WebContents) so that the WC
can then automatically set the accessibility state of any new frames that
are created in that tab.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[jam, 2014-05-09 22:25:19]

On Fri, May 9, 2014 at 3:22 PM, Dominic Mazzoni <dmazzoni@google.com> wrote:

> On Fri, May 9, 2014 at 3:15 PM, John Abd-El-Malek <jam@chromium.org>wrote:
>
>> If the accessibility code wants to enable a flag for all WC, where is
>> this code, in chrome or content? If in chrome, can it iterate across all
>> the tabs in all the browser objects?
>>
>
> There are two places we need this - in AccessibilityUI and
> BrowserAccessibilityStateImpl, both times the implementation is inside
> content - so would it be okay to have a way to enumerate all
> WebContentsImpls instead?
>

Yep, sure that is fine as it's very reasonable that content can iterate
over any of its own objects. We can't get layering violations that way.


> In both cases, I think it makes more sense for both of them to control the
> accessibility state of a whole tab (or other WebContents) so that the WC
> can then automatically set the accessibility state of any new frames that
> are created in that tab.
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-05-09 22:27:41]

On Fri, May 9, 2014 at 3:25 PM, John Abd-El-Malek <jam@chromium.org> wrote:

> Yep, sure that is fine as it's very reasonable that content can iterate
> over any of its own objects. We can't get layering violations that way.
>

Great, I'll start with a utility function that iterates over RWHs and
builds up a set of unique WCs from that - we can replace this with
something smarter if we need it, but then at least it's abstracted.

Thanks!

- Dominic

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-05-12 15:27:47]

OK, this change is almost ready - I need to rebase and fix a couple of
tests, but I'd appreciate a quick look to make sure the design seems solid
with respect to site isolation and modularity/layering.

  https://codereview.chromium.org/273423004/

We'll land this first, and then the cross-process iframe accessibility
change won't have to violate any layering constraints.

One thing I wanted to clarify: when there are multiple frames from the same
origin, will each one have a RenderFrame and RFH? Or will there only be a
separate set when it's a remote frame?

BTW, what's the correct address for site-isolation-reviews? My emails
bounced.

- Dominic

On Fri, May 9, 2014 at 3:27 PM, Dominic Mazzoni <dmazzoni@google.com> wrote:

> On Fri, May 9, 2014 at 3:25 PM, John Abd-El-Malek <jam@chromium.org>wrote:
>
>> Yep, sure that is fine as it's very reasonable that content can iterate
>> over any of its own objects. We can't get layering violations that way.
>>
>
> Great, I'll start with a utility function that iterates over RWHs and
> builds up a set of unique WCs from that - we can replace this with
> something smarter if we need it, but then at least it's abstracted.
>
> Thanks!
>
> - Dominic
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nasko, 2014-05-12 17:25:29]

On Mon, May 12, 2014 at 8:27 AM, Dominic Mazzoni <dmazzoni@google.com>wrote:

> OK, this change is almost ready - I need to rebase and fix a couple of
> tests, but I'd appreciate a quick look to make sure the design seems solid
> with respect to site isolation and modularity/layering.
>
>   https://codereview.chromium.org/273423004/
>

Looking at it.

We'll land this first, and then the cross-process iframe accessibility
> change won't have to violate any layering constraints.
>
> One thing I wanted to clarify: when there are multiple frames from the
> same origin, will each one have a RenderFrame and RFH? Or will there only
> be a separate set when it's a remote frame?
>

There will be a RenderFrame/RenderFrameHost for each frame, regardless of
which process it lives in. In current Chrome, even without
--site-per-process, this is true and you can see it on any page that has
iframes. It so happens that they all live in the same process right now.


> BTW, what's the correct address for site-isolation-reviews? My emails
> bounced.
>

Yeah, I had that happen. The email is correct, but you need to send from
your @chromium.org account.


> - Dominic
>
>
> On Fri, May 9, 2014 at 3:27 PM, Dominic Mazzoni <dmazzoni@google.com>wrote:
>
>> On Fri, May 9, 2014 at 3:25 PM, John Abd-El-Malek <jam@chromium.org>wrote:
>>
>>> Yep, sure that is fine as it's very reasonable that content can iterate
>>> over any of its own objects. We can't get layering violations that way.
>>>
>>
>> Great, I'll start with a utility function that iterates over RWHs and
>> builds up a set of unique WCs from that - we can replace this with
>> something smarter if we need it, but then at least it's abstracted.
>>
>> Thanks!
>>
>> - Dominic
>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-05-12 17:47:25]

On Mon, May 12, 2014 at 10:25 AM, Nasko Oskov <nasko@chromium.org> wrote:

>
>> One thing I wanted to clarify: when there are multiple frames from the
>> same origin, will each one have a RenderFrame and RFH? Or will there only
>> be a separate set when it's a remote frame?
>>
>
> There will be a RenderFrame/RenderFrameHost for each frame, regardless of
> which process it lives in. In current Chrome, even without
> --site-per-process, this is true and you can see it on any page that has
> iframes. It so happens that they all live in the same process right now.
>

Got it.

Okay, so currently in Blink the accessibility tree spans frames - so that
means that accessibility messages are only being generated and processed on
the root of the (local) frame tree. However, I don't think anything will
break - we'll just have some redundant code when there's local iframes.
Next I'll start refactoring the Blink code so that it creates a separate
accessibility tree for each frame independently, and exposes those via
WebFrame's delegate. At that point, RFH's accessibility code will make more
sense, and we'll be using the same codepaths whether frames are local or
remote.

- Dominic

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nasko, 2014-05-12 17:56:05]

On Mon, May 12, 2014 at 10:47 AM, Dominic Mazzoni <dmazzoni@google.com>wrote:

> On Mon, May 12, 2014 at 10:25 AM, Nasko Oskov <nasko@chromium.org> wrote:
>
>>
>>> One thing I wanted to clarify: when there are multiple frames from the
>>> same origin, will each one have a RenderFrame and RFH? Or will there only
>>> be a separate set when it's a remote frame?
>>>
>>
>> There will be a RenderFrame/RenderFrameHost for each frame, regardless of
>> which process it lives in. In current Chrome, even without
>> --site-per-process, this is true and you can see it on any page that has
>> iframes. It so happens that they all live in the same process right now.
>>
>
> Got it.
>
> Okay, so currently in Blink the accessibility tree spans frames - so that
> means that accessibility messages are only being generated and processed on
> the root of the (local) frame tree. However, I don't think anything will
> break - we'll just have some redundant code when there's local iframes.
> Next I'll start refactoring the Blink code so that it creates a separate
> accessibility tree for each frame independently, and exposes those via
> WebFrame's delegate. At that point, RFH's accessibility code will make more
> sense, and we'll be using the same codepaths whether frames are local or
> remote.
>

That sounds like a good plan. Once you split the tree for each frame, you
will need to stitch it together on the browser side so it looks unchanged
to everyone.
Thanks for being an early bird for this work!

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[chromium-reviews, 2014-05-12 18:03:49]

On Mon, May 12, 2014 at 10:56 AM, Nasko Oskov <nasko@chromium.org> wrote:

> Okay, so currently in Blink the accessibility tree spans frames - so that
>> means that accessibility messages are only being generated and processed on
>> the root of the (local) frame tree. However, I don't think anything will
>> break - we'll just have some redundant code when there's local iframes.
>> Next I'll start refactoring the Blink code so that it creates a separate
>> accessibility tree for each frame independently, and exposes those via
>> WebFrame's delegate. At that point, RFH's accessibility code will make more
>> sense, and we'll be using the same codepaths whether frames are local or
>> remote.
>>
>
> That sounds like a good plan. Once you split the tree for each frame, you
> will need to stitch it together on the browser side so it looks unchanged
> to everyone.
>

Yes, and to be clear, that's what we're trying to do already for
cross-process frames - but doing this for all frames will be a good way to
shake out the potential bugs early.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ncarter (slow), 2014-07-29 20:18:56]

Is this patch obsolete?

[dmazzoni, 2014-07-29 20:21:04]

No, this is the next step, but it needs a lot of work because the
refactoring changed a lot.



On Tue, Jul 29, 2014 at 1:18 PM, <nick@chromium.org> wrote:

> Is this patch obsolete?
>
> https://codereview.chromium.org/268543008/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2014-08-20 17:08:41]

Thanks for your patience! This is ready for a fresh look.

It depends on one tiny blink change: https://codereview.chromium.org/468723005/

[Charlie Reis, 2014-08-20 20:37:14]

Glad it's working now!  I'm having a bit of trouble following how this works,
though, so just a few clarifying questions for now.

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility.h (right):

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility.h:34: // Class implementing
the cross platform interface for the Browser-Renderer
Do we have a separate instance of this class for each FrameTreeNode?  I'm
unclear why it would have a frame ID member, since this comment makes it sound
like there's only one for the browser.

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility.h:248: // Identifies the
given frame id as the only child of this node, so
I don't follow why this would track a single child.  What if there were
multiple?

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility.h:252: void
SetChildFrameId(int64 child_frame_id);
nit: FrameId is ambiguous, between a RenderFrame routing ID and a FrameTreeNode
ID.  Can you clarify which one you mean, here and below?

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_manager.h (right):

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_manager.h:260: // frame's
root object.
This comment is kind of hard to follow.  Maybe it would be clearer with an
example?

[dmazzoni, 2014-08-20 20:57:18]

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility.h (right):

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility.h:34: // Class implementing
the cross platform interface for the Browser-Renderer
On 2014/08/20 20:37:13, Charlie Reis wrote:
> Do we have a separate instance of this class for each FrameTreeNode?  I'm
> unclear why it would have a frame ID member, since this comment makes it sound
> like there's only one for the browser.

Sorry, I haven't updated that comment in years. There's an instance of this for
practically every element on the page. I'll be renaming this AXPlatformNode
soon, and BrowserAccessibilityManager -> AXPlatformTree.

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility.h:248: // Identifies the
given frame id as the only child of this node, so
On 2014/08/20 20:37:13, Charlie Reis wrote:
> I don't follow why this would track a single child.  What if there were
> multiple?

This object would correspond to an actual <iframe> element on a page. It'd have
one child, the child frame.

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility.h:252: void
SetChildFrameId(int64 child_frame_id);
On 2014/08/20 20:37:13, Charlie Reis wrote:
> nit: FrameId is ambiguous, between a RenderFrame routing ID and a
FrameTreeNode
> ID.  Can you clarify which one you mean, here and below?

Agreed, let me clarify.

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_manager.h (right):

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_manager.h:260: // frame's
root object.
On 2014/08/20 20:37:13, Charlie Reis wrote:
> This comment is kind of hard to follow.  Maybe it would be clearer with an
> example?

Sure.

I'll work on the comment, but for purposes of your understanding the code
review:

Imagine frame P has an iframe element with node id 37, where that id is relative
to its frame.

The iframe's accessibility tree is this BrowserAccessibilityManager. This field
tells us the id - 37 - of the element that hosts this whole frame in the parent
frame.

This is needed so we can walk back up the tree. If the client requests the
parent of the root of this tree, we need to return the object id=37 from the
parent frame.

[dmazzoni, 2014-08-20 21:04:10]

On Wed, Aug 20, 2014 at 1:57 PM, <dmazzoni@chromium.org> wrote:

> AXPlatformNode soon, and BrowserAccessibilityManager -> AXPlatformTree.
>

Maybe it should be AXPlatformFrame or AXPlatformFrameTree, since there's
going to be one per frame now.

AXTree is a cross-platform tree of AXNodes representing the accessible view
of a frame. The "Platform" classes are parallel classes that implement the
native accessibility APIs for each platform on top of that. Joining
together the frames into a single composed tree is a concept that only
needs to exist at the "Platform" level - it's done on-demand. At every
other level of the system, the frames are mostly independent.

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Charlie Reis, 2014-08-21 19:23:28]

A few more comments as I start to follow it more.  The main questions are around
how we deal with RenderFrameProxies and whether we need to be careful about
process swaps that occur during the traversal (if that's possible).

It would have been nice to have the tree traversal occur over FrameTreeNodes
rather than RenderFrameHosts, but that would probably only make sense if BAM was
associated with a FTN rather than RFH, and that's probably not the right thing
to do.

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility.cc (right):

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility.cc:98:
child_manager->SetNodeIdInParentFrame(GetId());
Seems odd to be doing a modification to the child_manager as part of a const
GetFoo method.  Is there another place this could be set?

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_manager.cc (right):

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_manager.cc:73:
node_id_in_parent_frame_(0),
Let's not use a magic number here, since it's easy to confuse this with the -1
used elsewhere.

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_manager.h (right):

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_manager.h:33: const int
kNoFrameId = -1;
This is a bit ambiguous for a content-wide constant.  Beyond clarifying frame
routing ID vs FrameTreeNode ID vs node ID, we should probably add "Ax" in the
name.

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
File content/browser/accessibility/site_per_process_accessibility_browsertest.cc
(right):

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
content/browser/accessibility/site_per_process_accessibility_browsertest.cc:33:
// TODO(dmazzoni): share this with site_per_process_browsertest.cc
Can you subclass SitePerProcessBrowserTest, or is something like DEPS preventing
that?

We can abstract it out into useful test utilities if needed.

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
content/browser/accessibility/site_per_process_accessibility_browsertest.cc:119:
EXPECT_NE(shell()->web_contents()->GetRenderViewHost(), rvh);
Many of these checks are redundant with the CrossSiteIframe test.  We can just
verify that the frame has a different SiteInstance and move on to the
interesting accessibility checks.

https://codereview.chromium.org/268543008/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/268543008/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:464: RenderFrameHostImpl*
render_frame_host = node->current_frame_host();
Should we be concerned if a process swap occurs in one of these frames and the
current_frame_host is different than when we started?  (I'm not sure how
asynchronous this traversal is, or if it all takes place on one pass through the
event loop.)

It might be fine (e.g., we just get the BAM for the most recent frame), but I
want to be sure that doesn't break any assumptions.

https://codereview.chromium.org/268543008/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:970: // to notify out
BrowserAccessibilityManager of the frame tree node id of
nit: out -> our?

https://codereview.chromium.org/268543008/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:972: std::map<int32,
int>::const_iterator iter;
Can we add a comment about this map being from node IDs to frame routing IDs,
and whether it's purely RenderFrame IDs in the current process or also has
RenderFrameProxy IDs?

https://codereview.chromium.org/268543008/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:982: GetProcess()->GetID(),
frame_routing_id);
How does this work for iframes that are already out of process?  Wouldn't they
have RenderFrameProxies in the renderer process, such that the routing ID would
be a proxy and this would return NULL?

(I'm assuming that we're looping over a set of nodeID -> frameRoutingID pairs
from a given renderer process, and I'm wondering what happens when that process
has RenderFrameProxies.)

https://codereview.chromium.org/268543008/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/268543008/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:109: int64 frame_id)
OVERRIDE;
Should this be |frame_tree_node_id|?

https://codereview.chromium.org/268543008/diff/20001/content/common/accessibi...
File content/common/accessibility_messages.h (right):

https://codereview.chromium.org/268543008/diff/20001/content/common/accessibi...
content/common/accessibility_messages.h:60: // Mapping from node id to frame
routing id for an out-of-process iframe.
Are these frame routing IDs from RenderFrames or RenderFrameProxies?  (I would
expect the latter for OOPIFs, since we won't have RenderFrames for them.)

https://codereview.chromium.org/268543008/diff/20001/content/renderer/accessi...
File content/renderer/accessibility/blink_ax_tree_source.cc (right):

https://codereview.chromium.org/268543008/diff/20001/content/renderer/accessi...
content/renderer/accessibility/blink_ax_tree_source.cc:462: RenderFrameImpl*
render_frame = RenderFrameImpl::FromWebFrame(frame);
Seems like this would return NULL if frame were a WebRemoteFrame.

https://codereview.chromium.org/268543008/diff/20001/content/renderer/accessi...
File content/renderer/accessibility/blink_ax_tree_source.h (right):

https://codereview.chromium.org/268543008/diff/20001/content/renderer/accessi...
content/renderer/accessibility/blink_ax_tree_source.h:51: std::map<int32, int>*
frame_routing_ids_;
frame_routing_ids_ sounds like a list, without indicating that it's a map. 
Maybe something like node_to_frame_map_?

[dmazzoni, 2014-08-25 06:49:37]

Thanks for the great feedback!

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility.cc (right):

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility.cc:98:
child_manager->SetNodeIdInParentFrame(GetId());
On 2014/08/21 19:23:27, Charlie Reis wrote:
> Seems odd to be doing a modification to the child_manager as part of a const
> GetFoo method.  Is there another place this could be set?

I agree this is ugly.

Maybe this is premature optimization. The child frame can just search the parent
frame for the matching node each time. If that search shows up in a code profile
in the future I can always optimize it to cache that id later.

I'll delete node_id_in_parent_frame completely for now.

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility.h (right):

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility.h:34: // Class implementing
the cross platform interface for the Browser-Renderer
Comment updated a bit.

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility.h:252: void
SetChildFrameId(int64 child_frame_id);
On 2014/08/20 20:57:17, dmazzoni wrote:
> On 2014/08/20 20:37:13, Charlie Reis wrote:
> > nit: FrameId is ambiguous, between a RenderFrame routing ID and a
> FrameTreeNode
> > ID.  Can you clarify which one you mean, here and below?
> 
> Agreed, let me clarify.

Done.

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_manager.cc (right):

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_manager.cc:73:
node_id_in_parent_frame_(0),
On 2014/08/21 19:23:27, Charlie Reis wrote:
> Let's not use a magic number here, since it's easy to confuse this with the -1
> used elsewhere.

This is also gone now.

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_manager.h (right):

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_manager.h:33: const int
kNoFrameId = -1;
On 2014/08/21 19:23:27, Charlie Reis wrote:
> This is a bit ambiguous for a content-wide constant.  Beyond clarifying frame
> routing ID vs FrameTreeNode ID vs node ID, we should probably add "Ax" in the
> name.

WIth the other changes, this is only needed in browser_accessibility.cc now.
Frame tree node ids start at 1, so maybe we don't need a constant at all? I can
just document that it's only valid if nonzero.

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_manager.h:260: // frame's
root object.
This is gone now.

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
File content/browser/accessibility/site_per_process_accessibility_browsertest.cc
(right):

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
content/browser/accessibility/site_per_process_accessibility_browsertest.cc:33:
// TODO(dmazzoni): share this with site_per_process_browsertest.cc
On 2014/08/21 19:23:27, Charlie Reis wrote:
> Can you subclass SitePerProcessBrowserTest, or is something like DEPS
preventing
> that?
> 
> We can abstract it out into useful test utilities if needed.

Done.

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
content/browser/accessibility/site_per_process_accessibility_browsertest.cc:119:
EXPECT_NE(shell()->web_contents()->GetRenderViewHost(), rvh);
On 2014/08/21 19:23:27, Charlie Reis wrote:
> Many of these checks are redundant with the CrossSiteIframe test.  We can just
> verify that the frame has a different SiteInstance and move on to the
> interesting accessibility checks.

Sounds good.

I deleted the ones that seemed the most redundant. Let me know if there's more
you think is unnecessary.

https://codereview.chromium.org/268543008/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/268543008/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:464: RenderFrameHostImpl*
render_frame_host = node->current_frame_host();
On 2014/08/21 19:23:27, Charlie Reis wrote:
> Should we be concerned if a process swap occurs in one of these frames and the
> current_frame_host is different than when we started?  (I'm not sure how
> asynchronous this traversal is, or if it all takes place on one pass through
the
> event loop.)
> 
> It might be fine (e.g., we just get the BAM for the most recent frame), but I
> want to be sure that doesn't break any assumptions.

We're using this safely; when BrowserAccessibilityManager calls any function on
BrowserAccessibilityDelegate, it basically only assumes that return value is
valid within that call stack, it never caches it.

I should probably formalize that contract. I'll add a comment to
BrowserAccessibilityDelegate now.

https://codereview.chromium.org/268543008/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:970: // to notify out
BrowserAccessibilityManager of the frame tree node id of
On 2014/08/21 19:23:27, Charlie Reis wrote:
> nit: out -> our?

Done.

https://codereview.chromium.org/268543008/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:972: std::map<int32,
int>::const_iterator iter;
On 2014/08/21 19:23:27, Charlie Reis wrote:
> Can we add a comment about this map being from node IDs to frame routing IDs,
> and whether it's purely RenderFrame IDs in the current process or also has
> RenderFrameProxy IDs?

Done. I think it should allow RenderFrameProxy IDs.

https://codereview.chromium.org/268543008/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:982: GetProcess()->GetID(),
frame_routing_id);
On 2014/08/21 19:23:27, Charlie Reis wrote:
> How does this work for iframes that are already out of process?  Wouldn't they
> have RenderFrameProxies in the renderer process, such that the routing ID
would
> be a proxy and this would return NULL?
> 
> (I'm assuming that we're looping over a set of nodeID -> frameRoutingID pairs
> from a given renderer process, and I'm wondering what happens when that
process
> has RenderFrameProxies.)

Agreed, this should handle RenderFrameProxyHosts too. Done.

https://codereview.chromium.org/268543008/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/268543008/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.h:109: int64 frame_id)
OVERRIDE;
On 2014/08/21 19:23:27, Charlie Reis wrote:
> Should this be |frame_tree_node_id|?

Done.

https://codereview.chromium.org/268543008/diff/20001/content/common/accessibi...
File content/common/accessibility_messages.h (right):

https://codereview.chromium.org/268543008/diff/20001/content/common/accessibi...
content/common/accessibility_messages.h:60: // Mapping from node id to frame
routing id for an out-of-process iframe.
On 2014/08/21 19:23:27, Charlie Reis wrote:
> Are these frame routing IDs from RenderFrames or RenderFrameProxies?  (I would
> expect the latter for OOPIFs, since we won't have RenderFrames for them.)

Clarified.

I think it's useful to have both - you never know when a RenderFrame is going to
suddenly go out-of-process, and if we've already looked up the frame tree node
id on the browser side, we can work with it right away rather than waiting until
we get the notification of the RenderFrameProxy routing id.

https://codereview.chromium.org/268543008/diff/20001/content/renderer/accessi...
File content/renderer/accessibility/blink_ax_tree_source.cc (right):

https://codereview.chromium.org/268543008/diff/20001/content/renderer/accessi...
content/renderer/accessibility/blink_ax_tree_source.cc:462: RenderFrameImpl*
render_frame = RenderFrameImpl::FromWebFrame(frame);
On 2014/08/21 19:23:27, Charlie Reis wrote:
> Seems like this would return NULL if frame were a WebRemoteFrame.

Agreed. The test was passing only because we were converting the routing_id to a
frame_tree_node_id before the cross-site transition - once we have the
frame_tree_node_id we can follow the frame wherever it goes. We should handle
both cases here, though.

https://codereview.chromium.org/268543008/diff/20001/content/renderer/accessi...
File content/renderer/accessibility/blink_ax_tree_source.h (right):

https://codereview.chromium.org/268543008/diff/20001/content/renderer/accessi...
content/renderer/accessibility/blink_ax_tree_source.h:51: std::map<int32, int>*
frame_routing_ids_;
On 2014/08/21 19:23:27, Charlie Reis wrote:
> frame_routing_ids_ sounds like a list, without indicating that it's a map. 
> Maybe something like node_to_frame_map_?

Done.

[Charlie Reis, 2014-08-25 20:58:44]

This is looking much better!  A few nits and comments below, but I think this is
close.

Nasko, any thoughts on the addition of RenderFrameProxyHost::FromID?  It looks
right to me.

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
File content/browser/accessibility/site_per_process_accessibility_browsertest.cc
(right):

https://codereview.chromium.org/268543008/diff/20001/content/browser/accessib...
content/browser/accessibility/site_per_process_accessibility_browsertest.cc:119:
EXPECT_NE(shell()->web_contents()->GetRenderViewHost(), rvh);
On 2014/08/25 06:49:36, dmazzoni wrote:
> On 2014/08/21 19:23:27, Charlie Reis wrote:
> > Many of these checks are redundant with the CrossSiteIframe test.  We can
just
> > verify that the frame has a different SiteInstance and move on to the
> > interesting accessibility checks.
> 
> Sounds good.
> 
> I deleted the ones that seemed the most redundant. Let me know if there's more
> you think is unnecessary.

Looks good now, thanks.

https://codereview.chromium.org/268543008/diff/40001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_manager.h (right):

https://codereview.chromium.org/268543008/diff/40001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_manager.h:46: // Note:
BrowserAccessibilityManager should never cache any of the return
Nice.

https://codereview.chromium.org/268543008/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/268543008/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:990: RenderFrameProxyHost*
child = RenderFrameProxyHost::FromID(
nit: Let's not shadow the previous |child|.  |proxy| or |child_proxy| would do.

https://codereview.chromium.org/268543008/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_proxy_host.h (right):

https://codereview.chromium.org/268543008/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.h:74: static
RenderFrameProxyHost* FromID(int process_id, int routing_id);
nit: Static method should be first, before the constructor.

https://codereview.chromium.org/268543008/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.h:105: FrameTreeNode*
frame_tree_node() const { return frame_tree_node_; };
nit: This one's fairly fundamental, so let's list it higher up, maybe near
GetSiteInstance().

https://codereview.chromium.org/268543008/diff/40001/content/browser/site_per...
File content/browser/site_per_process_browsertest.cc (left):

https://codereview.chromium.org/268543008/diff/40001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:197:
IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, DISABLED_CrossSiteIframe) {
Ah, you might need to rebase past Nasko's CL at some point.

https://codereview.chromium.org/268543008/diff/40001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:364:
DISABLED_CrossSiteIframeRedirectOnce) {
These probably shouldn't be re-enabled.

https://codereview.chromium.org/268543008/diff/40001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/268543008/diff/40001/content/content_tests.gy...
content/content_tests.gypi:1167: 'browser/site_per_process_browsertest.h',
I think you forgot to add this file to the patchset.

[nasko, 2014-08-25 21:23:30]

Adding RenderFrameProxyHost::FromID is expected, we just didn't need it before,
so totally fine with me. Couple of nits from a cursory drive-by.

https://codereview.chromium.org/268543008/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/268543008/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.cc:39:
g_routing_id_frame_proxy_map.Get().insert(std::make_pair(
nit: I like checking the return value of insert, just to make sure we don't have
any bugs creep up on us.

https://codereview.chromium.org/268543008/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.cc:131: RenderFrameProxyHost*
RenderFrameProxyHost::FromID(int process_id,
nit: static methods should be at the top of the file

[dmazzoni, 2014-08-26 23:31:03]

Ready for another look.

As soon as blink 180911 gets rolled I'll start a try job.

https://codereview.chromium.org/268543008/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/268543008/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:990: RenderFrameProxyHost*
child = RenderFrameProxyHost::FromID(
On 2014/08/25 20:58:43, Charlie Reis wrote:
> nit: Let's not shadow the previous |child|.  |proxy| or |child_proxy| would
do.

Done.

https://codereview.chromium.org/268543008/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/268543008/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.cc:39:
g_routing_id_frame_proxy_map.Get().insert(std::make_pair(
On 2014/08/25 21:23:29, nasko wrote:
> nit: I like checking the return value of insert, just to make sure we don't
have
> any bugs creep up on us.

Is this what you had in mind?

  DCHECK(g_routing_id_frame_proxy_map.Get().insert(
      std::make_pair(
          RenderFrameProxyHostID(GetProcess()->GetID(), routing_id_),
          this)).second);

https://codereview.chromium.org/268543008/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.cc:131: RenderFrameProxyHost*
RenderFrameProxyHost::FromID(int process_id,
On 2014/08/25 21:23:29, nasko wrote:
> nit: static methods should be at the top of the file

Done.

https://codereview.chromium.org/268543008/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_proxy_host.h (right):

https://codereview.chromium.org/268543008/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.h:74: static
RenderFrameProxyHost* FromID(int process_id, int routing_id);
On 2014/08/25 20:58:43, Charlie Reis wrote:
> nit: Static method should be first, before the constructor.

Done.

https://codereview.chromium.org/268543008/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.h:105: FrameTreeNode*
frame_tree_node() const { return frame_tree_node_; };
On 2014/08/25 20:58:43, Charlie Reis wrote:
> nit: This one's fairly fundamental, so let's list it higher up, maybe near
> GetSiteInstance().

Done.

https://codereview.chromium.org/268543008/diff/40001/content/browser/site_per...
File content/browser/site_per_process_browsertest.cc (left):

https://codereview.chromium.org/268543008/diff/40001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:197:
IN_PROC_BROWSER_TEST_F(SitePerProcessBrowserTest, DISABLED_CrossSiteIframe) {
On 2014/08/25 20:58:44, Charlie Reis wrote:
> Ah, you might need to rebase past Nasko's CL at some point.

Yes, it works again now.

https://codereview.chromium.org/268543008/diff/40001/content/browser/site_per...
content/browser/site_per_process_browsertest.cc:364:
DISABLED_CrossSiteIframeRedirectOnce) {
On 2014/08/25 20:58:43, Charlie Reis wrote:
> These probably shouldn't be re-enabled.

Oops, forgot I left these in.

https://codereview.chromium.org/268543008/diff/40001/content/content_tests.gypi
File content/content_tests.gypi (right):

https://codereview.chromium.org/268543008/diff/40001/content/content_tests.gy...
content/content_tests.gypi:1167: 'browser/site_per_process_browsertest.h',
On 2014/08/25 20:58:44, Charlie Reis wrote:
> I think you forgot to add this file to the patchset.

Done.

[Charlie Reis, 2014-08-27 17:28:35]

Cool, thanks.

I put in a little more thought about the places where we are traversing or
searching the frame tree, since these are likely going to be used by many
features.  I made some suggestions below, but let me know if those prove to be
problematic.

Otherwise, I think we're about set.  Thanks for your work on this!

https://codereview.chromium.org/268543008/diff/40001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/268543008/diff/40001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.cc:39:
g_routing_id_frame_proxy_map.Get().insert(std::make_pair(
On 2014/08/26 23:31:03, dmazzoni wrote:
> On 2014/08/25 21:23:29, nasko wrote:
> > nit: I like checking the return value of insert, just to make sure we don't
> have
> > any bugs creep up on us.
> 
> Is this what you had in mind?
> 
>   DCHECK(g_routing_id_frame_proxy_map.Get().insert(
>       std::make_pair(
>           RenderFrameProxyHostID(GetProcess()->GetID(), routing_id_),
>           this)).second);
> 

You'll need to store the return value and DCHECK(result.second), since the
bodies of DCHECKs get compiled out.  For example:
https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr...

https://codereview.chromium.org/268543008/diff/80001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_manager.cc (right):

https://codereview.chromium.org/268543008/diff/80001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_manager.cc:393: static
BrowserAccessibility* FindParentOfNode(
If this is a nonmember function, it should probably be at the top in an unnamed
namespace and doesn't need static, right?

https://codereview.chromium.org/268543008/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/268543008/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:470: FrameTreeNode* node =
frame_tree->FindByID(frame_tree_node_id);
This will probably do for now, but it would be nice to think about whether
there's a more elegant way to walk the FrameTree in parallel with the
accessibility tree.

At the moment, we're linearly searching the FrameTree every time we encounter an
OOPIF in the accessibility tree, making it O(n^2), where n is the number of
OOPIFs in the FrameTree.  (It's not O(n log n) because it's not a binary
search.)  We should be able to walk the two trees in parallel visiting each node
once, in O(n) time.

I'm not terribly concerned about this, given that n (and even the size of the
FrameTree including same-site frames) is likely to be very small.  Still, we
should come up with a better pattern for similar features to follow.

At the very least, I just noticed that we already have
FrameTree::GloballyFindByID with a global hash_map of frames.  That would give
us an O(1) lookup without even needing to go through this RFH's FrameTree.

https://codereview.chromium.org/268543008/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:994: RenderFrameHostImpl*
child = RenderFrameHostImpl::FromID(
Here's another block that FrameTree should be providing: a way to look up a
FrameTreeNode given a process ID and routing ID, whether it's a RenderFrameHost
or a RenderFrameProxyHost.

In fact, we're already most of the way there.  FrameTree::FindByRoutingID has a
TODO to search the "swapped out RFHs" as well, which today means searching the
RenderFrameProxyHosts.  Can we update and use that, since we expect future
callers to need it as well?

(We can probably make that efficient using the two RFH/RFPH FromID methods and
then double-checking that the result actually belongs to the FrameTree in
question.)

https://codereview.chromium.org/268543008/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/268543008/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.cc:49:
DCHECK(g_routing_id_frame_proxy_map.Get().insert(
This will get compiled out in release builds.

[dmazzoni, 2014-08-28 05:40:14]

https://codereview.chromium.org/268543008/diff/80001/content/browser/accessib...
File content/browser/accessibility/browser_accessibility_manager.cc (right):

https://codereview.chromium.org/268543008/diff/80001/content/browser/accessib...
content/browser/accessibility/browser_accessibility_manager.cc:393: static
BrowserAccessibility* FindParentOfNode(
On 2014/08/27 17:28:34, Charlie Reis wrote:
> If this is a nonmember function, it should probably be at the top in an
unnamed
> namespace and doesn't need static, right?

Done.

https://codereview.chromium.org/268543008/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/268543008/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:470: FrameTreeNode* node =
frame_tree->FindByID(frame_tree_node_id);
On 2014/08/27 17:28:34, Charlie Reis wrote:
> This will probably do for now, but it would be nice to think about whether
> there's a more elegant way to walk the FrameTree in parallel with the
> accessibility tree.
> 
> At the moment, we're linearly searching the FrameTree every time we encounter
an
> OOPIF in the accessibility tree, making it O(n^2), where n is the number of
> OOPIFs in the FrameTree.  (It's not O(n log n) because it's not a binary
> search.)  We should be able to walk the two trees in parallel visiting each
node
> once, in O(n) time.

Oh, I never would have guessed that it wasn't storing that in a hash map. Is
there any reason not to just maintain a hash map from frame tree node id to
frame tree node, local to each frame tree?

> I'm not terribly concerned about this, given that n (and even the size of the
> FrameTree including same-site frames) is likely to be very small.  Still, we
> should come up with a better pattern for similar features to follow.
> 
> At the very least, I just noticed that we already have
> FrameTree::GloballyFindByID with a global hash_map of frames.  That would give
> us an O(1) lookup without even needing to go through this RFH's FrameTree.

Good thought.

I switched to using GloballyFindByID followed by a check that we got a node in
the same frame tree. That look better?

https://codereview.chromium.org/268543008/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:994: RenderFrameHostImpl*
child = RenderFrameHostImpl::FromID(
On 2014/08/27 17:28:35, Charlie Reis wrote:
> Here's another block that FrameTree should be providing: a way to look up a
> FrameTreeNode given a process ID and routing ID, whether it's a
RenderFrameHost
> or a RenderFrameProxyHost.
> 
> In fact, we're already most of the way there.  FrameTree::FindByRoutingID has
a
> TODO to search the "swapped out RFHs" as well, which today means searching the
> RenderFrameProxyHosts.  Can we update and use that, since we expect future
> callers to need it as well?
> 
> (We can probably make that efficient using the two RFH/RFPH FromID methods and
> then double-checking that the result actually belongs to the FrameTree in
> question.)

Good idea! Is this what you had in mind?

https://codereview.chromium.org/268543008/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/268543008/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.cc:49:
DCHECK(g_routing_id_frame_proxy_map.Get().insert(
On 2014/08/27 17:28:35, Charlie Reis wrote:
> This will get compiled out in release builds.

Ha! Good catch.

Think it should be a CHECK, or should I save the result and DCHECK it (ugly)?

[Charlie Reis, 2014-08-28 18:18:52]

Fantastic.  LGTM with the comments below!

https://codereview.chromium.org/268543008/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/268543008/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:470: FrameTreeNode* node =
frame_tree->FindByID(frame_tree_node_id);
On 2014/08/28 05:40:14, dmazzoni wrote:
> On 2014/08/27 17:28:34, Charlie Reis wrote:
> > This will probably do for now, but it would be nice to think about whether
> > there's a more elegant way to walk the FrameTree in parallel with the
> > accessibility tree.
> > 
> > At the moment, we're linearly searching the FrameTree every time we
encounter
> an
> > OOPIF in the accessibility tree, making it O(n^2), where n is the number of
> > OOPIFs in the FrameTree.  (It's not O(n log n) because it's not a binary
> > search.)  We should be able to walk the two trees in parallel visiting each
> node
> > once, in O(n) time.
> 
> Oh, I never would have guessed that it wasn't storing that in a hash map. Is
> there any reason not to just maintain a hash map from frame tree node id to
> frame tree node, local to each frame tree?

We were adding tree accessors as they were needed, and the use cases were mainly
traversals where the map wouldn't help much.  A more recent use case needed the
global lookup.

Now that we have the global map, we could probably change FrameTree::FindByID to
use it (returning null if the result is from a different tree).  No need to do
that in this CL unless you're feeling particularly proactive.

> 
> > I'm not terribly concerned about this, given that n (and even the size of
the
> > FrameTree including same-site frames) is likely to be very small.  Still, we
> > should come up with a better pattern for similar features to follow.
> > 
> > At the very least, I just noticed that we already have
> > FrameTree::GloballyFindByID with a global hash_map of frames.  That would
give
> > us an O(1) lookup without even needing to go through this RFH's FrameTree.
> 
> Good thought.
> 
> I switched to using GloballyFindByID followed by a check that we got a node in
> the same frame tree. That look better?

Yes, that's great.  The CHECK is fine if we never expect to pass in a value from
a different tree, but if an exploited renderer could confuse us, then we should
probably just return null.

https://codereview.chromium.org/268543008/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_impl.cc:994: RenderFrameHostImpl*
child = RenderFrameHostImpl::FromID(
On 2014/08/28 05:40:14, dmazzoni wrote:
> On 2014/08/27 17:28:35, Charlie Reis wrote:
> > Here's another block that FrameTree should be providing: a way to look up a
> > FrameTreeNode given a process ID and routing ID, whether it's a
> RenderFrameHost
> > or a RenderFrameProxyHost.
> > 
> > In fact, we're already most of the way there.  FrameTree::FindByRoutingID
has
> a
> > TODO to search the "swapped out RFHs" as well, which today means searching
the
> > RenderFrameProxyHosts.  Can we update and use that, since we expect future
> > callers to need it as well?
> > 
> > (We can probably make that efficient using the two RFH/RFPH FromID methods
and
> > then double-checking that the result actually belongs to the FrameTree in
> > question.)
> 
> Good idea! Is this what you had in mind?

Perfect!

https://codereview.chromium.org/268543008/diff/80001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/268543008/diff/80001/content/browser/frame_ho...
content/browser/frame_host/render_frame_proxy_host.cc:49:
DCHECK(g_routing_id_frame_proxy_map.Get().insert(
On 2014/08/28 05:40:14, dmazzoni wrote:
> On 2014/08/27 17:28:35, Charlie Reis wrote:
> > This will get compiled out in release builds.
> 
> Ha! Good catch.
> 
> Think it should be a CHECK, or should I save the result and DCHECK it (ugly)?

A CHECK is fine here.  We have a similar CHECK in RFHM::SwapOutOldPage where we
use a saved result, but we could skip the saved result there.

https://codereview.chromium.org/268543008/diff/100001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/268543008/diff/100001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:114: CHECK_EQ(this,
result->frame_tree());
Thinking this over, we may want to return NULL in this case instead of crashing,
since I think an exploited renderer process could send us a routing ID for the
wrong tab to cause the browser to crash.  Same below.

https://codereview.chromium.org/268543008/diff/140001/content/browser/frame_h...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/268543008/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:24: // The (process id,
routing id) pair that identifies one RenderFrame.
nit: RenderFrame -> RenderFrameProxy

[dmazzoni, 2014-09-05 06:16:21]

https://codereview.chromium.org/268543008/diff/100001/content/browser/frame_h...
File content/browser/frame_host/frame_tree.cc (right):

https://codereview.chromium.org/268543008/diff/100001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:114: CHECK_EQ(this,
result->frame_tree());
On 2014/08/28 18:18:52, Charlie Reis (OOO until Sep 8) wrote:
> Thinking this over, we may want to return NULL in this case instead of
crashing,
> since I think an exploited renderer process could send us a routing ID for the
> wrong tab to cause the browser to crash.  Same below.

Done.

[dmazzoni, 2014-09-05 06:18:19]

https://codereview.chromium.org/268543008/diff/140001/content/browser/frame_h...
File content/browser/frame_host/render_frame_proxy_host.cc (right):

https://codereview.chromium.org/268543008/diff/140001/content/browser/frame_h...
content/browser/frame_host/render_frame_proxy_host.cc:24: // The (process id,
routing id) pair that identifies one RenderFrame.
On 2014/08/28 18:18:52, Charlie Reis (OOO until Sep 8) wrote:
> nit: RenderFrame -> RenderFrameProxy

Done.

[dmazzoni, 2014-09-05 13:28:40]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-09-05 13:30:48]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/268543008/320001

[commit-bot: I haz the power, 2014-09-05 13:47:49]

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

[commit-bot: I haz the power, 2014-09-05 13:47:50]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[nasko, 2014-09-05 16:29:02]

LGTM with one nit

https://codereview.chromium.org/268543008/diff/320001/content/browser/frame_h...
File content/browser/frame_host/render_frame_host_impl.cc (right):

https://codereview.chromium.org/268543008/diff/320001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1011: for (unsigned int i =
0; i < params.size(); ++i) {
Let's use size_t instead of unsigned int here.

[dmazzoni, 2014-09-05 19:22:31]

Argh, the underlying site-per-process stuff seems to be broken on trunk,
but the test that would have caught it is still disabled on all platforms
(SitePerProcessBrowserTest.CrossSiteIframe)

As of r293457 (Thu Sep 4 22:52:52), my test passed
As of r293551 (Fri Sep 5 10:49:34), it fails - NavigateFrameToURL on the
cross-site iframe doesn't seem to succeed.



On Fri, Sep 5, 2014 at 9:29 AM, <nasko@chromium.org> wrote:

> LGTM with one nit
>
>
> https://codereview.chromium.org/268543008/diff/320001/
> content/browser/frame_host/render_frame_host_impl.cc
> File content/browser/frame_host/render_frame_host_impl.cc (right):
>
> https://codereview.chromium.org/268543008/diff/320001/
> content/browser/frame_host/render_frame_host_impl.cc#newcode1011
> content/browser/frame_host/render_frame_host_impl.cc:1011: for (unsigned
> int i = 0; i < params.size(); ++i) {
> Let's use size_t instead of unsigned int here.
>
> https://codereview.chromium.org/268543008/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[nasko, 2014-09-08 14:18:53]

I've landed https://codereview.chromium.org/543273002/, which should
restore sanity to CrossSiteIframe test and yours (though I haven't
explicitly patched it in to confirm).


On Fri, Sep 5, 2014 at 12:22 PM, Dominic Mazzoni <dmazzoni@chromium.org>
wrote:

> Argh, the underlying site-per-process stuff seems to be broken on trunk,
> but the test that would have caught it is still disabled on all platforms
> (SitePerProcessBrowserTest.CrossSiteIframe)
>
> As of r293457 (Thu Sep 4 22:52:52), my test passed
> As of r293551 (Fri Sep 5 10:49:34), it fails - NavigateFrameToURL on the
> cross-site iframe doesn't seem to succeed.
>
>
>
> On Fri, Sep 5, 2014 at 9:29 AM, <nasko@chromium.org> wrote:
>
>> LGTM with one nit
>>
>>
>> https://codereview.chromium.org/268543008/diff/320001/
>> content/browser/frame_host/render_frame_host_impl.cc
>> File content/browser/frame_host/render_frame_host_impl.cc (right):
>>
>> https://codereview.chromium.org/268543008/diff/320001/
>> content/browser/frame_host/render_frame_host_impl.cc#newcode1011
>> content/browser/frame_host/render_frame_host_impl.cc:1011: for (unsigned
>> int i = 0; i < params.size(); ++i) {
>> Let's use size_t instead of unsigned int here.
>>
>> https://codereview.chromium.org/268543008/
>>
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2014-09-08 15:08:21]

Thanks! Trying again now...


On Mon, Sep 8, 2014 at 7:18 AM, Nasko Oskov <nasko@chromium.org> wrote:

> I've landed https://codereview.chromium.org/543273002/, which should
> restore sanity to CrossSiteIframe test and yours (though I haven't
> explicitly patched it in to confirm).
>
>
> On Fri, Sep 5, 2014 at 12:22 PM, Dominic Mazzoni <dmazzoni@chromium.org>
> wrote:
>
>> Argh, the underlying site-per-process stuff seems to be broken on trunk,
>> but the test that would have caught it is still disabled on all platforms
>> (SitePerProcessBrowserTest.CrossSiteIframe)
>>
>> As of r293457 (Thu Sep 4 22:52:52), my test passed
>> As of r293551 (Fri Sep 5 10:49:34), it fails - NavigateFrameToURL on the
>> cross-site iframe doesn't seem to succeed.
>>
>>
>>
>> On Fri, Sep 5, 2014 at 9:29 AM, <nasko@chromium.org> wrote:
>>
>>> LGTM with one nit
>>>
>>>
>>> https://codereview.chromium.org/268543008/diff/320001/
>>> content/browser/frame_host/render_frame_host_impl.cc
>>> File content/browser/frame_host/render_frame_host_impl.cc (right):
>>>
>>> https://codereview.chromium.org/268543008/diff/320001/
>>> content/browser/frame_host/render_frame_host_impl.cc#newcode1011
>>> content/browser/frame_host/render_frame_host_impl.cc:1011: for (unsigned
>>> int i = 0; i < params.size(); ++i) {
>>> Let's use size_t instead of unsigned int here.
>>>
>>> https://codereview.chromium.org/268543008/
>>>
>>
>>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[dmazzoni, 2014-09-08 15:12:43]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-09-08 15:14:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/268543008/340001

[dmazzoni, 2014-09-08 15:54:41]

The CQ bit was unchecked by dmazzoni@chromium.org

[dmazzoni, 2014-09-09 22:02:30]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-09-09 22:06:10]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/268543008/360001

[commit-bot: I haz the power, 2014-09-10 00:09:07]

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

[commit-bot: I haz the power, 2014-09-10 00:09:08]

Exceeded time limit waiting for builds to trigger.

[dmazzoni, 2014-09-10 05:34:59]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-09-10 05:36:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/268543008/360001

[commit-bot: I haz the power, 2014-09-10 06:11:01]

Committed patchset #19 (id:360001) as 5a9cdbdffd9e534f0945d0b160b6dfee4cdee4d2

[commit-bot: I haz the power, 2014-09-10 06:14:55]

Patchset 19 (id:??) landed as
https://crrev.com/387942c041da17ea6337bc0a81e96619e67e4ac4
Cr-Commit-Position: refs/heads/master@{#294118}

[dconnelly, 2014-09-10 07:42:43]

A revert of this CL (patchset #19 id:360001) has been created in
https://codereview.chromium.org/558943002/ by dconnelly@chromium.org.

The reason for reverting is: Broke Mac tests:

http://build.chromium.org/p/chromium.mac/builders/Mac10.7%20Tests%20%281%29/b...

http://build.chromium.org/p/chromium.mac/builders/Mac%2010.6%20Tests%20%28dbg...

http://build.chromium.org/p/chromium.mac/builders/Mac%2010.7%20Tests%20%28dbg...

http://build.chromium.org/p/chromium.mac/builders/Mac%2010.7%20Tests%20%28dbg...

and so on..

[dmazzoni, 2014-09-10 16:26:22]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-09-10 16:26:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/268543008/400001

[commit-bot: I haz the power, 2014-09-10 16:37:34]

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

[commit-bot: I haz the power, 2014-09-10 16:37:36]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/54089)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/59037)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dmazzoni, 2014-09-10 16:42:42]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-09-10 16:43:57]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/268543008/420001

[commit-bot: I haz the power, 2014-09-10 19:48:41]

Committed patchset #22 (id:420001) as a5434786bd3063082ce16e67ef3f1e9ce71cab19

[commit-bot: I haz the power, 2014-09-10 19:53:39]

Patchset 22 (id:??) landed as
https://crrev.com/0b5d2481a5dc4f3a1ed812b174a4da8dde1b64c2
Cr-Commit-Position: refs/heads/master@{#294210}