Issue 1542743002: [RDHI] Refactored blocked_loaders_map_ to key by render frame route id

Charlie Harrison

Description was changed from ========== Refactored blocked_loaders_map_ to key by render frame route id This ...

5 years ago (2015-12-21 20:10:08 UTC) #1

Charlie Harrison

Description was changed from ========== Refactored blocked_loaders_map_ to key by render frame route id This ...

4 years, 12 months ago (2015-12-26 21:40:59 UTC) #2

Charlie Harrison

csharrison@chromium.org changed reviewers: + davidben@chromium.org, nasko@chromium.org, ncarter@chromium.org, rdsmith@chromium.org

4 years, 12 months ago (2015-12-28 15:37:31 UTC) #3

Charlie Harrison

I'd love some high level comments on this change. Some comments of my own: - ...

4 years, 12 months ago (2015-12-28 15:37:32 UTC) #4

Randy Smith (Not in Mondays)

On 2015/12/28 15:37:32, csharrison wrote: > I'd love some high level comments on this change. ...

4 years, 11 months ago (2015-12-29 23:04:45 UTC) #5

On 2015/12/28 15:37:32, csharrison wrote:
> I'd love some high level comments on this change.

High level comments (here and inline):
* You might want to take a look at https://codereview.chromium.org/1529363006. 
I haven't done the review yet (:-}), but I think Lukasz is implementing id type
safety in a way we might want to emulate at this level.
* It looks like you've added LoaderIOThreadNotifier without actually using it
anywhere; that correct?  I generally oppose adding infrastructure before first
use.

> Some comments of my own:
> - I'm not sure of GlobalFrameRoutingID as a name. If it stays, I will change
ID
> => Id as nasko@ mentioned in a previous CL.

No strong feelings on naming.

> - I would like to put GlobalFrameRoutingId in content/public/ and change all
the
> interfaces to RDH to use it rather than paired opaque ints. This would be nice
> with a new method on RFH GetGlobalRoutingId(). If that's the only place in the
> code we instantiate the struct, we get pretty nice type safety.

So this gets at my biggest worry looking at this change, which is that it
strikes me that it would be pretty easy to be passing a RV route id as an RF
route id without realizing it.  If you can find some way to dynamically (or
better yet, statically) assert that that hasn't happened, I'd be much more
comfortable (== I'd feel like I don't need to do quite as thorough a review,
which may not matter to you, but does to me ;-}).  So I'd strongly support
*something* like what you describe above, though I'm neutral as to what.

> 
> - I'm willing to make more code go through the ui to io notifier. So far
that's
> a bit of a shell class until we want to notify ResourceScheduler of more UI
> events. Many thread hops in this CL could go through static methods there.
> 
>
https://codereview.chromium.org/1542743002/diff/140001/content/public/test/mo...
> File content/public/test/mock_render_process_host.cc (right):
> 
>
https://codereview.chromium.org/1542743002/diff/140001/content/public/test/mo...
> content/public/test/mock_render_process_host.cc:260: void
> MockRenderProcessHost::ResumeRequestsForFrame(int route_id) {}
> Will fix {} in next patch.

Randy Smith (Not in Mondays)

Whoops, also had little comments as I read through. https://codereview.chromium.org/1542743002/diff/140001/content/browser/loader/global_routing_id.h File content/browser/loader/global_routing_id.h (right): https://codereview.chromium.org/1542743002/diff/140001/content/browser/loader/global_routing_id.h#newcode45 content/browser/loader/global_routing_id.h:45: ...

4 years, 11 months ago (2015-12-29 23:11:37 UTC) #6

Charlie Harrison

Addressed some inline comments. I think in a later patch I'll put the routing type ...

4 years, 11 months ago (2015-12-30 20:51:49 UTC) #7

Addressed some inline comments.

I think in a later patch I'll put the routing type in public/

Eventually, I think we'll use Lukasz's id type for routing ids, but for now I
think it's best to encapsulate it in GlobalFrameRoutingID. Will defer to content
owners on that though.

https://codereview.chromium.org/1542743002/diff/140001/content/browser/loader...
File content/browser/loader/global_routing_id.h (right):

https://codereview.chromium.org/1542743002/diff/140001/content/browser/loader...
content/browser/loader/global_routing_id.h:45: : GlobalRoutingID(child_id,
render_frame_route_id) {}
On 2015/12/29 23:11:36, Randy Smith - OOO until M 1-4 wrote:
> Any way to assert that?  Don't we know something about the run time value of
the
> frame routing ids?

Hm I'm not quite sure. There was discussion on the site-isolation dev list about
making them distinct from RVIDs at runtime. I think that idea was superseded by
the ID type work.

https://codereview.chromium.org/1542743002/diff/140001/content/browser/loader...
File content/browser/loader/loader_io_thread_notifier.cc (right):

https://codereview.chromium.org/1542743002/diff/140001/content/browser/loader...
content/browser/loader/loader_io_thread_notifier.cc:21:
DCHECK_CURRENTLY_ON(BrowserThread::IO);
On 2015/12/29 23:11:37, Randy Smith - OOO until M 1-4 wrote:
> I'd be more aggressive on scattering these through this file; they're very
> useful for documentation.

Done.

https://codereview.chromium.org/1542743002/diff/140001/content/browser/loader...
content/browser/loader/loader_io_thread_notifier.cc:70:
base::Bind(&NotifyForEachFrameOnIOHelper, frame_callback, routing_ids));
On 2015/12/29 23:11:37, Randy Smith - OOO until M 1-4 wrote:
> nit, suggestion: This looks like a lot of copying; I'm tempted to suggest you
> allocate the set once, and pass a scoped_ptr<>.

Done.

https://codereview.chromium.org/1542743002/diff/140001/content/browser/loader...
File content/browser/loader/loader_io_thread_notifier.h (right):

https://codereview.chromium.org/1542743002/diff/140001/content/browser/loader...
content/browser/loader/loader_io_thread_notifier.h:22: class
LoaderIOThreadNotifer : public WebContentsObserver {
On 2015/12/29 23:11:37, Randy Smith - OOO until M 1-4 wrote:
> A comment specifying the thread behavior of the class/the static method within
> it?  I'm guessing it's pretty simple, but it's also pretty core to how the
class
> behaves, so I think it would be a plus.

Done.

https://codereview.chromium.org/1542743002/diff/140001/content/browser/loader...
File content/browser/loader/resource_request_info_impl.cc (right):

https://codereview.chromium.org/1542743002/diff/140001/content/browser/loader...
content/browser/loader/resource_request_info_impl.cc:333: int render_frame_id,
On 2015/12/29 23:11:37, Randy Smith - OOO until M 1-4 wrote:
> Good catch.  (== Ooops, I should have caught this when I did the CL to make
RRII
> always have good frame ids.)

No prob!

https://codereview.chromium.org/1542743002/diff/140001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1542743002/diff/140001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1920: void
RenderProcessHostImpl::ResumeRequestsForFrame(int route_id) {
On 2015/12/29 23:11:37, Randy Smith - OOO until M 1-4 wrote:
> I hope you were really careful about making sure this transformation
propagated
> up and down the callstack from here, since the compiler wouldn't have said a
> word :-}.

The approach I did here (and other places) was to change "ForRoute" or "ForView"
to "ForFrame", forcing all callers to change. I hoped that would catch all
cases, but your comment that the "compiler wouldn't have said a word" makes me
worried.

https://codereview.chromium.org/1542743002/diff/140001/content/browser/render...
File content/browser/renderer_host/render_widget_host_impl.cc (left):

https://codereview.chromium.org/1542743002/diff/140001/content/browser/render...
content/browser/renderer_host/render_widget_host_impl.cc:401: 
On 2015/12/29 23:11:37, Randy Smith - OOO until M 1-4 wrote:
> Why?

I moved this logic to RenderFrameHost::Init(). I could have had this method
resume requests for the main frame of the widget, though.

https://codereview.chromium.org/1542743002/diff/140001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1542743002/diff/140001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:412:
loader_io_thread_notifier_.reset(new LoaderIOThreadNotifer(this));
On 2015/12/29 23:11:37, Randy Smith - OOO until M 1-4 wrote:
> I'm confused; naively, it looks like you're never using this object after you
> construct it.

The only way this gets used is as a WebContentsObserver (passing |this| starts
observing). I included it as a scoped pointer here for lifetime reasons. I
wasn't sure if there was a better way to do this. It also might be useful later
if WCO methods need to call methods directly (too specialized for WCO
callbacks).

https://codereview.chromium.org/1542743002/diff/140001/content/public/test/mo...
File content/public/test/mock_render_process_host.cc (right):

https://codereview.chromium.org/1542743002/diff/140001/content/public/test/mo...
content/public/test/mock_render_process_host.cc:260: void
MockRenderProcessHost::ResumeRequestsForFrame(int route_id) {}
On 2015/12/28 15:37:32, csharrison wrote:
> Will fix {} in next patch.

Done.

Randy Smith (Not in Mondays)

Responses to comments inline. Given that you asked for high level comments, I'm assuming that ...

4 years, 11 months ago (2016-01-04 21:24:48 UTC) #8

Responses to comments inline.  Given that you asked for high level comments, I'm
assuming that you're good with what I've given you for now--let me know if you
want another round of review.

https://codereview.chromium.org/1542743002/diff/140001/content/browser/loader...
File content/browser/loader/global_routing_id.h (right):

https://codereview.chromium.org/1542743002/diff/140001/content/browser/loader...
content/browser/loader/global_routing_id.h:45: : GlobalRoutingID(child_id,
render_frame_route_id) {}
On 2015/12/30 20:51:49, csharrison wrote:
> On 2015/12/29 23:11:36, Randy Smith - OOO until M 1-4 wrote:
> > Any way to assert that?  Don't we know something about the run time value of
> the
> > frame routing ids?
> 
> Hm I'm not quite sure. There was discussion on the site-isolation dev list
about
> making them distinct from RVIDs at runtime. I think that idea was superseded
by
> the ID type work.

Drat; I thought I remembered someone telling me that there were run-time
differences between frame and view ids (even vs. odd) but scanning the code that
doesn't seem to be the case; they both seem to be allocated off
RenderProcessHostImpl::GetNextRoutingId().  So I guess we don't have a good way
to do the assertion :-J.

https://codereview.chromium.org/1542743002/diff/140001/content/browser/render...
File content/browser/renderer_host/render_process_host_impl.cc (right):

https://codereview.chromium.org/1542743002/diff/140001/content/browser/render...
content/browser/renderer_host/render_process_host_impl.cc:1920: void
RenderProcessHostImpl::ResumeRequestsForFrame(int route_id) {
On 2015/12/30 20:51:49, csharrison wrote:
> On 2015/12/29 23:11:37, Randy Smith - OOO until M 1-4 wrote:
> > I hope you were really careful about making sure this transformation
> propagated
> > up and down the callstack from here, since the compiler wouldn't have said a
> > word :-}.
> 
> The approach I did here (and other places) was to change "ForRoute" or
"ForView"
> to "ForFrame", forcing all callers to change. I hoped that would catch all
> cases, but your comment that the "compiler wouldn't have said a word" makes me
> worried.

No, actually, that sounds like a good approach.  Sorry to make you worried;
thanks for sketching out your method.

https://codereview.chromium.org/1542743002/diff/140001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1542743002/diff/140001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:412:
loader_io_thread_notifier_.reset(new LoaderIOThreadNotifer(this));
On 2015/12/30 20:51:49, csharrison wrote:
> On 2015/12/29 23:11:37, Randy Smith - OOO until M 1-4 wrote:
> > I'm confused; naively, it looks like you're never using this object after
you
> > construct it.
> 
> The only way this gets used is as a WebContentsObserver (passing |this| starts
> observing). I included it as a scoped pointer here for lifetime reasons. I
> wasn't sure if there was a better way to do this. It also might be useful
later
> if WCO methods need to call methods directly (too specialized for WCO
> callbacks).

Right, whoops, sorry I missed that.  I'm good with this.

Charlie Harrison

So I ended up refactoring this a bit. Some changes: 1. RDHI interface now takes ...

4 years, 11 months ago (2016-01-04 22:07:43 UTC) #9

So I ended up refactoring this a bit. Some changes:

1. RDHI interface now takes in GlobalFrameRoutingId. This gives us a bit more
type safety and better method signatures for API consumers.

2. Most places construct a GlobalFrameRoutingId from a RenderFrameHostImpl.
Those that don't do not have access to one (and only have the routes / proc
ids).

3. All RDHI tasks posted to the IO thread now go through the
LoaderIOThreadNotifier(Impl). I decided to do this because I'm nervous about
some cases where people have been calling ResourceDispatcherHostImpl::Get()
before the thread hop (possible race condition).

PTAL.

https://codereview.chromium.org/1542743002/diff/140001/content/browser/loader...
File content/browser/loader/global_routing_id.h (right):

https://codereview.chromium.org/1542743002/diff/140001/content/browser/loader...
content/browser/loader/global_routing_id.h:45: : GlobalRoutingID(child_id,
render_frame_route_id) {}
On 2016/01/04 at 21:24:48, Randy Smith - OOO until M 1-4 wrote:
> On 2015/12/30 20:51:49, csharrison wrote:
> > On 2015/12/29 23:11:36, Randy Smith - OOO until M 1-4 wrote:
> > > Any way to assert that?  Don't we know something about the run time value
of
> > the
> > > frame routing ids?
> > 
> > Hm I'm not quite sure. There was discussion on the site-isolation dev list
about
> > making them distinct from RVIDs at runtime. I think that idea was superseded
by
> > the ID type work.
> 
> Drat; I thought I remembered someone telling me that there were run-time
differences between frame and view ids (even vs. odd) but scanning the code that
doesn't seem to be the case; they both seem to be allocated off
RenderProcessHostImpl::GetNextRoutingId().  So I guess we don't have a good way
to do the assertion :-J.

I think this was done as a local hack a few times, but I don't think it was ever
landed.

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.h:216: // Causes all new
requests for the route identified by |child_id| and
Will fix this comment in next patch.

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.h:391: // acts like
CancelRequestsForProcess when route_id is -1.
Will fix this comment in the next patch.

Randy Smith (Not in Mondays)

So my primary focus in this review is on the architecture of LoaderIOThreadNotifier, and I ...

4 years, 11 months ago (2016-01-06 21:56:30 UTC) #10

So my primary focus in this review is on the architecture of
LoaderIOThreadNotifier, and I have a couple of discomforts in that space:

* The base class isn't an interface (== abstract base class), but has no state
and no non-static methods.  That effectively makes it a container for functions.
 I really don't think that it's worthwhile adding something to the content/
interface for that purpose; I'd just put those functions on RDH as static
methods.

* I have the same thought (though not as strongly; there isn't a class being
created for it) about LoadIOThreadNotifierImpl's static methods; I think they'd
be better off as static methods (with appropriate commenting about thread homes)
on RDHI.  This narrows its functionality down to what I think of as its main
purpose in life: Forwarding WebContentsObserver notifications from UI to IO
threads. 

* Given that LoadIOThreadNotifierImpl is tightly tied to the RDHI, I'm inclined
to suggest it be in the RDHI header file (optional: As a nested class of RDHI).

* I'm uncomfortable (as I think I mentioned in some of the detailed comments
below) about the reliance on ResourceDispatcherHostImpl::Get(); I'd rather see
the class initialized with that value so that we're just dependent on the global
in one place rather than every time we call.  I don't think this brings in any
lifetime issues, as I think if this functionality can be used after RDHI is
destroyed we're in trouble anyway.  Having said that, use of RDHI::Get() is all
around in the code, so this is a suggestion.

* The methods currently on LIOTNI seem like a bit of a grab bag, presumably
because you've just included the ones that are currently needed in the code. 
One way to deal with this is to flesh out in comments the expansion pathway for
this class and what interfaces would be appropriate on it and what not as it
grows (there's a detailed comment below that's a specialization of this general
thought).

Another alternative that I'm not sure it worthwhile, but I'll sketch out for you
to decide what you think: make it a bit more general by allowing it to forward
any WebContentsObserver notifications to a WebContentsObserver (or maybe a
derived WebContentsObserverIO) which happens to live on the IO thread.  There
are a couple of issues with this approach: 
a) You'd have to take my above suggestion around the RDHI being passed to the
constructor, and would need RDHI to inherit from a base WebContentsObserverIO
class, 
b) It brings the lifetime issues front-and-center, and probably the only way to
deal with them generally is weak pointers (either using base::WeakPtr<> or just
having WebContentsObserverIO having a stub class that's owned across both
threads and does forwarding to the actual WCOIO, though thinking that
implementation through I think a straight weak pointer is better), and 
c) if you didn't put some cleverness in that I don't have a blinding insight for
off the top of my head you'd be forwarding a lot of notifications you don't care
about, which is a performance minus.  But doing it would provide a cleaner, more
modular interface.  I'm just not sure it's worth it, so I'll raise the thought
and let you decide whether you like it or not :-}.

https://codereview.chromium.org/1542743002/diff/200001/content/browser/frame_...
File content/browser/frame_host/interstitial_page_impl.cc (right):

https://codereview.chromium.org/1542743002/diff/200001/content/browser/frame_...
content/browser/frame_host/interstitial_page_impl.cc:842: //
LoaderIOThreadNotifier tests if there is a ResourceDispatcherHostImpl, as
This comment seems wrong to me--LIOTN isn't testing and the test is being done
here.  Unupdated comment?

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
File content/browser/loader/loader_io_thread_notifier_impl.cc (right):

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
content/browser/loader/loader_io_thread_notifier_impl.cc:20: bool
CollectRenderFrameRoutingIds(std::set<GlobalFrameRoutingId>* route_ids,
I'd value a quick comment explaining the goal of this call.  When I started
reading it I assumed it was to collect all frame routing ids for an entire tree,
but that doesn't look like it's goal based on the implementation.

Putting it near the function that it's a helper for would also be useful.  I
don't think the style guide prohibits putting static functions anywhere in a
file.

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
content/browser/loader/loader_io_thread_notifier_impl.cc:21: FrameTreeNode*
tree_node) {
Style guide says parameter order is inputs, then outputs:
https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
content/browser/loader/loader_io_thread_notifier_impl.cc:22:
RenderFrameHostImpl* frame_host = tree_node->current_frame_host();
DCHECK for thread?  I think it's useful being a-r about that in this file.

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
content/browser/loader/loader_io_thread_notifier_impl.cc:39:
ResourceDispatcherHostImpl* rdh = ResourceDispatcherHostImpl::Get();
What are the lifetime implications here?  Are you sure that there's no way for
the RDHI to be torn down in a race with this message, and if so, why?  (Probably
worthwhile documenting the rationale somewhere.)

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
content/browser/loader/loader_io_thread_notifier_impl.cc:49: const
GlobalFrameRoutingId& routing_id) {
idea (up to you): I find myself wondering about whether implementing the above
function in terms of this function might be reasonable for code re-use.  It's
right at the edge for me (which is why I'm not pushing it).

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
File content/browser/loader/loader_io_thread_notifier_impl.h (right):

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
content/browser/loader/loader_io_thread_notifier_impl.h:24: // thread.
nit: Remove unneeded newline?

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
content/browser/loader/loader_io_thread_notifier_impl.h:36: // explicitly take a
RenderFrameHost*.
Hmmm.  I find this comment a little confusing.  The only other method here that
takes an RFH is RenderFrameDeleted.  Are you saying "Create new methods that
take a RFH in preference to using this if the RFH is available?"

https://codereview.chromium.org/1542743002/diff/200001/content/browser/render...
File content/browser/renderer_host/render_widget_helper.cc (left):

https://codereview.chromium.org/1542743002/diff/200001/content/browser/render...
content/browser/renderer_host/render_widget_helper.cc:156: 
When are the requests blocked above resumed?

https://codereview.chromium.org/1542743002/diff/200001/content/browser/render...
File content/browser/renderer_host/render_widget_helper.cc (right):

https://codereview.chromium.org/1542743002/diff/200001/content/browser/render...
content/browser/renderer_host/render_widget_helper.cc:117:
GlobalFrameRoutingId(render_process_id_, *main_frame_route_id));
This is safe because at this point there will only be one frame in the window? 
If so, maybe a comment to that effect?

https://codereview.chromium.org/1542743002/diff/200001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1542743002/diff/200001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1821:
LoaderIOThreadNotifierImpl::ResumeBlockedRequestsForFrameInternal(
Help me understand the context in which this code path is taken?  The existence
of this interface is a wart (IMO) and I'd love to find some way around needing
it.

https://codereview.chromium.org/1542743002/diff/200001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1821:
LoaderIOThreadNotifierImpl::ResumeBlockedRequestsForFrameInternal(
I presume that it's safe to just target the frame rather than the view here
because this view won't have had a chance to create more frames at this point in
the code?  Worth documenting that if so.

nasko

Some preliminary comments, still going through the CL and only reviewed half of it. https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader/global_routing_id.h ...

4 years, 11 months ago (2016-01-07 00:43:09 UTC) #11

nasko

Few more comments. https://codereview.chromium.org/1542743002/diff/200001/content/browser/browser_plugin/browser_plugin_guest.cc File content/browser/browser_plugin/browser_plugin_guest.cc (right): https://codereview.chromium.org/1542743002/diff/200001/content/browser/browser_plugin/browser_plugin_guest.cc#newcode794 content/browser/browser_plugin/browser_plugin_guest.cc:794: GetWebContents()->GetMainFrame()->Init(); Uh, this is unfortunate. Can ...

4 years, 11 months ago (2016-01-07 19:58:40 UTC) #12

Charlie Harrison

Thank you both for the reviews. I ended up putting the static methods in ResourceDispatcherHost/Impl, ...

4 years, 11 months ago (2016-01-07 22:47:06 UTC) #13

Thank you both for the reviews. I ended up putting the static methods in
ResourceDispatcherHost/Impl, along with the LoaderIOThreadNotifier. I also
reparented this CL on top of one that just updates
ResourceRequestInfo::UpdateForTransfer.

Randy: I don't think I want to write an IOWebContentsObserver. I don't think
it's necessary. If the notifier class ends up needing more structure we can come
back to this general idea.

Nasko: Still trying to figure out a way to remove that added call to
RenderFrameHost::Init().

https://codereview.chromium.org/1542743002/diff/200001/content/browser/browse...
File content/browser/browser_plugin/browser_plugin_guest.cc (right):

https://codereview.chromium.org/1542743002/diff/200001/content/browser/browse...
content/browser/browser_plugin/browser_plugin_guest.cc:794:
GetWebContents()->GetMainFrame()->Init();
On 2016/01/07 19:58:39, nasko wrote:
> Uh, this is unfortunate. Can we avoid calling this explicitly?

Yeah this is extremely ugly. All the original code has resuming done in the
RenderWidgetHostImpl::Init()

Having that method resume for the main frame seems ugly too, though.

What do you thing the best approach is?

https://codereview.chromium.org/1542743002/diff/200001/content/browser/frame_...
File content/browser/frame_host/interstitial_page_impl.cc (right):

https://codereview.chromium.org/1542743002/diff/200001/content/browser/frame_...
content/browser/frame_host/interstitial_page_impl.cc:842: //
LoaderIOThreadNotifier tests if there is a ResourceDispatcherHostImpl, as
On 2016/01/06 at 21:56:29, Randy Smith - Not in Fridays wrote:
> This comment seems wrong to me--LIOTN isn't testing and the test is being done
here.  Unupdated comment?

This code tests that we have a rvh. The LIOTN tests for RDH.

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
File content/browser/loader/global_routing_id.h (right):

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
content/browser/loader/global_routing_id.h:43: struct GlobalFrameRoutingId :
public GlobalRoutingID {
On 2016/01/07 at 00:43:09, nasko wrote:
> Is it possible to avoid the inheritance? It will allow us to use the compiler
to enforce type safety and as it is, this looks dangerous.

Yeah it's possible, we just need to copy the boilerplate. Done.

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
File content/browser/loader/loader_io_thread_notifier_impl.cc (right):

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
content/browser/loader/loader_io_thread_notifier_impl.cc:20: bool
CollectRenderFrameRoutingIds(std::set<GlobalFrameRoutingId>* route_ids,
On 2016/01/06 at 21:56:29, Randy Smith - Not in Fridays wrote:
> I'd value a quick comment explaining the goal of this call.  When I started
reading it I assumed it was to collect all frame routing ids for an entire tree,
but that doesn't look like it's goal based on the implementation.
> 
> Putting it near the function that it's a helper for would also be useful.  I
don't think the style guide prohibits putting static functions anywhere in a
file.

That is the goal, we just don't want to miss any (including pending /
speculative ones). Will comment this.

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
content/browser/loader/loader_io_thread_notifier_impl.cc:21: FrameTreeNode*
tree_node) {
On 2016/01/06 at 21:56:29, Randy Smith - Not in Fridays wrote:
> Style guide says parameter order is inputs, then outputs:
https://google.github.io/styleguide/cppguide.html#Function_Parameter_Ordering

I'm not sure the best way to reconcile this. FrameTree:ForEach requires a
Callback<bool(FrameTreeNode*)>. The currying rules wrt Bind require you to curry
in one direction (i.e. I have to specify route_ids first cause that's the arg
I'm currying away). LMK if you see a simple solution to this (ideally we'd use
an inline lambda and close over route_ids but I don't think they play nice with
Bind).

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
content/browser/loader/loader_io_thread_notifier_impl.cc:22:
RenderFrameHostImpl* frame_host = tree_node->current_frame_host();
On 2016/01/06 at 21:56:30, Randy Smith - Not in Fridays wrote:
> DCHECK for thread?  I think it's useful being a-r about that in this file.

Done.

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
content/browser/loader/loader_io_thread_notifier_impl.cc:39:
ResourceDispatcherHostImpl* rdh = ResourceDispatcherHostImpl::Get();
On 2016/01/06 21:56:29, Randy Smith - Not in Fridays wrote:
> What are the lifetime implications here?  Are you sure that there's no way for
> the RDHI to be torn down in a race with this message, and if so, why? 
(Probably
> worthwhile documenting the rationale somewhere.)

Well if RDHI is torn down then won't Get() return nullptr? In that case this
function will be a no op. The other way some consumers did it was to call Get()
before the hop, so use-after-frees were possible.

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
content/browser/loader/loader_io_thread_notifier_impl.cc:49: const
GlobalFrameRoutingId& routing_id) {
On 2016/01/06 at 21:56:29, Randy Smith - Not in Fridays wrote:
> idea (up to you): I find myself wondering about whether implementing the above
function in terms of this function might be reasonable for code re-use.  It's
right at the edge for me (which is why I'm not pushing it).

Yeah sgtm.

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
content/browser/loader/loader_io_thread_notifier_impl.cc:103:
base::Bind(&ResourceDispatcherHostImpl::OnRenderFrameHostDeleted),
On 2016/01/07 19:58:39, nasko wrote:
> This isn't quite correct. RenderFrame is the renderer side object and this
> notification tells you that this one was deleted. RenderFrameHost can persist
> around, so technically it isn't deleted. Let's try to be precise.

Fixed the name to refer to the RenderFrame.

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
File content/browser/loader/loader_io_thread_notifier_impl.h (right):

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
content/browser/loader/loader_io_thread_notifier_impl.h:24: // thread.
On 2016/01/06 at 21:56:30, Randy Smith - Not in Fridays wrote:
> nit: Remove unneeded newline?

Done.

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
content/browser/loader/loader_io_thread_notifier_impl.h:36: // explicitly take a
RenderFrameHost*.
On 2016/01/06 at 21:56:30, Randy Smith - Not in Fridays wrote:
> Hmmm.  I find this comment a little confusing.  The only other method here
that takes an RFH is RenderFrameDeleted.  Are you saying "Create new methods
that take a RFH in preference to using this if the RFH is available?"

I was referring to the non-impl static methods. I'll fix this.

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1861:
CancelRequestsForFrame(GlobalFrameRoutingId(child_id, -1 /* cancel all */));
On 2016/01/07 19:58:39, nasko wrote:
> -1 is not a valid routing id. MSG_ROUTING_NONE?
Done.

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.h:216: // Causes all new
requests for the route identified by |child_id| and
On 2016/01/04 at 22:07:43, csharrison wrote:
> Will fix this comment in next patch.

Done.

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.h:391: // acts like
CancelRequestsForProcess when route_id is -1.
On 2016/01/04 at 22:07:43, csharrison wrote:
> Will fix this comment in the next patch.

Done.

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
File content/browser/loader/resource_request_info_impl.cc (right):

https://codereview.chromium.org/1542743002/diff/200001/content/browser/loader...
content/browser/loader/resource_request_info_impl.cc:340: render_frame_id_ =
render_frame_id;
On 2016/01/07 19:58:39, nasko wrote:
> It looks like the changes in this file can be split out in a separate, much
> simpler CL.

Yep it can. I'll do that.

https://codereview.chromium.org/1542743002/diff/200001/content/browser/render...
File content/browser/renderer_host/render_widget_helper.cc (left):

https://codereview.chromium.org/1542743002/diff/200001/content/browser/render...
content/browser/renderer_host/render_widget_helper.cc:156: 
On 2016/01/06 21:56:30, Randy Smith - Not in Fridays wrote:
> When are the requests blocked above resumed?

Supplemented the comment above with this info.

https://codereview.chromium.org/1542743002/diff/200001/content/browser/render...
File content/browser/renderer_host/render_widget_helper.cc (right):

https://codereview.chromium.org/1542743002/diff/200001/content/browser/render...
content/browser/renderer_host/render_widget_helper.cc:117:
GlobalFrameRoutingId(render_process_id_, *main_frame_route_id));
On 2016/01/06 21:56:30, Randy Smith - Not in Fridays wrote:
> This is safe because at this point there will only be one frame in the window?

> If so, maybe a comment to that effect?

Done.

https://codereview.chromium.org/1542743002/diff/200001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1542743002/diff/200001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1821:
LoaderIOThreadNotifierImpl::ResumeBlockedRequestsForFrameInternal(
On 2016/01/06 21:56:30, Randy Smith - Not in Fridays wrote:
> I presume that it's safe to just target the frame rather than the view here
> because this view won't have had a chance to create more frames at this point
in
> the code?  Worth documenting that if so.

Done.

https://codereview.chromium.org/1542743002/diff/200001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1821:
LoaderIOThreadNotifierImpl::ResumeBlockedRequestsForFrameInternal(
On 2016/01/06 at 21:56:30, Randy Smith - Not in Fridays wrote:
> Help me understand the context in which this code path is taken?  The
existence of this interface is a wart (IMO) and I'd love to find some way around
needing it.

It's rather confusing :/ but I believe what is happening is that we need to
block requests that'll end up in new windows until the frame is ready and
initialized. This bit of code gets run when the WebContents decides it doesn't
want to be responsible for the new window (and creating a new WebContents).
Thus, it resumes requests and lets its delegate decide what to do (this happens
in ShouldCreateWebContents).

This is my thinking of what can happen:
1. The delegate will create one with the given IDs. This should go through a
similar path wrt creating a new WebContents (same process / route).
2. The delegate will do some other logic (like nothing, or open a new tab,
etc.). In these cases, it seems like the requests should not be blocked, but
cancelled. Because the view is never created, my thinking is that these requests
just sit in the RDHI blocked_loaders_map_. This is arguably better than
servicing the requests, but still seems wrong.

Some preliminary investigation for whether we can remove this now that NPAPI is
gone started out promising, but I tested a CL that does it here:
https://codereview.chromium.org/1567903002
and it looks like WebView depends on it. I'm going to keep it in the meantime
until that gets sorted out.

https://codereview.chromium.org/1542743002/diff/200001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1822:
GlobalFrameRoutingId(GetRenderViewHost()->GetProcess()->GetID(),
On 2016/01/07 at 00:43:09, nasko wrote:
> Why are you getting the process id from the RenderViewHost? It is the
RenderFrameHost one, right?

Good catch. Actually, I think we should be grabbing the process from
source_site_instance (if we're opening in same process). So to fix these
problems, I think the new code should be

if (site_instance == source_site_instance) {
  ResumeBlockedRequestsForFrameInternal(GlobalFrameRoutingId(render_process_id,
main_frame_id));
}

https://codereview.chromium.org/1542743002/diff/200001/extensions/browser/app...
File extensions/browser/app_window/app_window_contents.cc (right):

https://codereview.chromium.org/1542743002/diff/200001/extensions/browser/app...
extensions/browser/app_window/app_window_contents.cc:134:
content::LoaderIOThreadNotifier::BlockRequestsForFrame(rfh);
On 2016/01/07 00:43:09, nasko wrote:
> Can we stick those methods on an existing class that is a bit easier to
> understand? Looking from outside of content, I have very little idea what the
> loader notifier is and shouldn't really care much that there is IO thread
> involved in this.
> 
> Leaving the method on RDH seems better to me or even putting it on
> RenderFrameHost itself.

Yeah, I think I'll merge the LoaderIOThreadNotifier into RDHI.

nasko

Sending the comments I have since I looked at patch set 14 earlier this week. ...

4 years, 11 months ago (2016-01-16 00:15:58 UTC) #14

nasko

Yay, done reviewing this one! https://codereview.chromium.org/1542743002/diff/320001/content/browser/loader/global_routing_id.h File content/browser/loader/global_routing_id.h (right): https://codereview.chromium.org/1542743002/diff/320001/content/browser/loader/global_routing_id.h#newcode41 content/browser/loader/global_routing_id.h:41: // Same as GlobalRoutingID ...

4 years, 11 months ago (2016-01-20 22:22:10 UTC) #15

Randy Smith (Not in Mondays)

I'm still chewing on the architecture of RDH/LIOTN. On the bright side, once we nail ...

4 years, 11 months ago (2016-01-20 23:23:19 UTC) #16

I'm still chewing on the architecture of RDH/LIOTN.  On the bright side, once we
nail that down, I doubt I'll have much more to say on this CL :-}.

Preface for context: I have the sense that you're making the choice to have all
the UI thread interfaces be static and all of the non-UI thread interfaces be
regular methods.  In the abstract I think that's a good choice--it makes it very
clear what thread the data members live on, while keeping related functionality
together on the class.  But it has some implications that I've poked at in the
following.  

* Given the static<->UI thread choice, I think that the NotifyFor* interfaces
are a bit of a cheat, since they actually target/require knowledge of IO thread
interfaces on the UI thread, even though those interfaces aren't actually called
on the UI thread.  I'm of the opinion that the overall interface would be
cleaner if we instead had UI thread interfaces {Block,
{Resume,Cancel}Blocked}RequestsForFrameTree.  I think the set of interfaces that
would result in having on RDHI would be more coherent and easier for new users
of the class to grok; the Notify* interfaces force people into using function
pointers (and class method function pointers, which are even a bit more
confusing), but don't really provide a lot of extra power for that--there are
only a couple of options.  Let's just give the consumer those options. 

As an API simplification (up to you), I could imagine those interfaces being
"ForFrame" with a boolean indicating whether recursion from that frame was
desired, or we could simply make them always recurse (presuming you can get the
descendants of a frame from a frame, and presuming that the times when we're
targeting a single frame that frame is known to have no children) so that we
don't need separate interfaces/boolean argument to distinguish ForFrame and
ForFrameTree.

* I'd suggest that if we're going to make the UI/IO "appropriate API" division,
we restrict ourselves to using UI objects on the UI thread and routes on the IO
thread just for simplification of the API.  So this would mean that
WebContentsImpl::CreateNewWindow would call ResumeBlockedRequestsForFrame (not
*Route).  LoaderIOThreadNotifer doesn't count; it's effectively an appendage
class of ResourceDispatcherHostImpl.

* (Suggestion) At this point, LoaderIOThreadNotifier isn't really doing
much--it's just forwarding UI thread notifications to UI thread interfaces on
the RDHI.  I think I'd find the overall interface cleaner if that class was a
private class for RDHI (declared and defined in the .cc file) and there was a
(static) method on RDHI that was called when a WebContents was created that
attached a class instance to the contents.  That keeps the new header
declarations visible to a minimum.

(I wanted to find a way to make RDHI a WCO, but it's gotta observer a *lot* of
WCOs, and the WCO interface isn't built for that.  Which I think is a flaw in
the interface, but hey, what so I know :-}, and I'm certainly not going to ask
you to fix it.)

* The UI Thread->static choice also results in the pattern of declaring static
methods on the RDH abstract base class that are functionally equivalent to how
interfaces on abstract base classes are used (to delegate to the derived class).
 In other words, it's *almost* something that people are very familiar with and
used to, but not *quite*.  And the implementation only works because of the use
of the global accessor (RDHI::Get()).  I dislike RDHI::Get(), but it's used all
over the place, and it'd be real mission creep for this CL to push you to fix
it, so I won't :-}.  Having said that, I don't like perpetuating it, and writing
an interface that requires it feels like a perpetuation to me.  The combination
of those two discomforts makes me want to push you to make these non-static pure
virtual methods that are implemented in RDHI.  But that violates the
architectural invariant I think you're trying to setup, so I'm torn--I see why
you're doing what you're doing, but every time I look at that pattern I wince. 
We may want to talk about this in person.  (As an alternative path, if Matt is
neutral to positive about your pattern, I would yield to his arch sense.)

https://codereview.chromium.org/1542743002/diff/260001/content/browser/frame_...
File content/browser/frame_host/interstitial_page_impl.cc (right):

https://codereview.chromium.org/1542743002/diff/260001/content/browser/frame_...
content/browser/frame_host/interstitial_page_impl.cc:821: // have one.
I think this comment isn't needed anymore.

https://codereview.chromium.org/1542743002/diff/260001/content/browser/frame_...
content/browser/frame_host/interstitial_page_impl.cc:840:
rvh->GetDelegate()->GetFrameTree(), frame_callback);
Suggestion: IMO, this code would be simpler and more understandable if it just
called NotifyForEachFrame with the constructed callback from each branch of the
switch statement.

https://codereview.chromium.org/1542743002/diff/260001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (left):

https://codereview.chromium.org/1542743002/diff/260001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1744:
scheduler_->OnClientDeleted(child_id, route_id);
I thought this was going to be removed; planning to yank it in a different CL?

https://codereview.chromium.org/1542743002/diff/260001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/1542743002/diff/260001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:461: FrameTreeNode*
tree_node) {
It looks to me like tree_node is purely an input argument, so it could be const
(even a const&), and I think by the style guide it should go before the output
arguments.

https://codereview.chromium.org/1542743002/diff/260001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/1542743002/diff/260001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.h:41: #include
"content/public/browser/render_view_host.h"
Why?

https://codereview.chromium.org/1542743002/diff/260001/content/browser/web_co...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1542743002/diff/260001/content/browser/web_co...
content/browser/web_contents/web_contents_impl.cc:1842: // source.
The second sentence "feels" to me as if it's an implementation dependency of an
implementation well removed from this code.  Am I intuiting correctly?  If so, I
think it's an unwise thing to rely on (and hence shouldn't be in the comment).

Charlie Harrison

On 2016/01/20 23:23:19, Randy Smith - Not in Fridays wrote: > I'm still chewing on ...

4 years, 11 months ago (2016-01-21 00:56:06 UTC) #17

On 2016/01/20 23:23:19, Randy Smith - Not in Fridays wrote:
> I'm still chewing on the architecture of RDH/LIOTN.  On the bright side, once
we
> nail that down, I doubt I'll have much more to say on this CL :-}.
> 
> Preface for context: I have the sense that you're making the choice to have
all
> the UI thread interfaces be static and all of the non-UI thread interfaces be
> regular methods.  In the abstract I think that's a good choice--it makes it
very
> clear what thread the data members live on, while keeping related
functionality
> together on the class.  But it has some implications that I've poked at in the
> following.  
> 
> * Given the static<->UI thread choice, I think that the NotifyFor* interfaces
> are a bit of a cheat, since they actually target/require knowledge of IO
thread
> interfaces on the UI thread, even though those interfaces aren't actually
called
> on the UI thread.  I'm of the opinion that the overall interface would be
> cleaner if we instead had UI thread interfaces {Block,
> {Resume,Cancel}Blocked}RequestsForFrameTree.  I think the set of interfaces
that
> would result in having on RDHI would be more coherent and easier for new users
> of the class to grok; the Notify* interfaces force people into using function
> pointers (and class method function pointers, which are even a bit more
> confusing), but don't really provide a lot of extra power for that--there are
> only a couple of options.  Let's just give the consumer those options. 
> 
> As an API simplification (up to you), I could imagine those interfaces being
> "ForFrame" with a boolean indicating whether recursion from that frame was
> desired, or we could simply make them always recurse (presuming you can get
the
> descendants of a frame from a frame, and presuming that the times when we're
> targeting a single frame that frame is known to have no children) so that we
> don't need separate interfaces/boolean argument to distinguish ForFrame and
> ForFrameTree.
> 
> * I'd suggest that if we're going to make the UI/IO "appropriate API"
division,
> we restrict ourselves to using UI objects on the UI thread and routes on the
IO
> thread just for simplification of the API.  So this would mean that
> WebContentsImpl::CreateNewWindow would call ResumeBlockedRequestsForFrame (not
> *Route).  LoaderIOThreadNotifer doesn't count; it's effectively an appendage
> class of ResourceDispatcherHostImpl.
> 
> * (Suggestion) At this point, LoaderIOThreadNotifier isn't really doing
> much--it's just forwarding UI thread notifications to UI thread interfaces on
> the RDHI.  I think I'd find the overall interface cleaner if that class was a
> private class for RDHI (declared and defined in the .cc file) and there was a
> (static) method on RDHI that was called when a WebContents was created that
> attached a class instance to the contents.  That keeps the new header
> declarations visible to a minimum.
> 
> (I wanted to find a way to make RDHI a WCO, but it's gotta observer a *lot* of
> WCOs, and the WCO interface isn't built for that.  Which I think is a flaw in
> the interface, but hey, what so I know :-}, and I'm certainly not going to ask
> you to fix it.)
> 
> * The UI Thread->static choice also results in the pattern of declaring static
> methods on the RDH abstract base class that are functionally equivalent to how
> interfaces on abstract base classes are used (to delegate to the derived
class).
>  In other words, it's *almost* something that people are very familiar with
and
> used to, but not *quite*.  And the implementation only works because of the
use
> of the global accessor (RDHI::Get()).  I dislike RDHI::Get(), but it's used
all
> over the place, and it'd be real mission creep for this CL to push you to fix
> it, so I won't :-}.  Having said that, I don't like perpetuating it, and
writing
> an interface that requires it feels like a perpetuation to me.  The
combination
> of those two discomforts makes me want to push you to make these non-static
pure
> virtual methods that are implemented in RDHI.  But that violates the
> architectural invariant I think you're trying to setup, so I'm torn--I see why
> you're doing what you're doing, but every time I look at that pattern I wince.

> We may want to talk about this in person.  (As an alternative path, if Matt is
> neutral to positive about your pattern, I would yield to his arch sense.)
> 
>
https://codereview.chromium.org/1542743002/diff/260001/content/browser/frame_...
> File content/browser/frame_host/interstitial_page_impl.cc (right):
> 
>
https://codereview.chromium.org/1542743002/diff/260001/content/browser/frame_...
> content/browser/frame_host/interstitial_page_impl.cc:821: // have one.
> I think this comment isn't needed anymore.
> 
>
https://codereview.chromium.org/1542743002/diff/260001/content/browser/frame_...
> content/browser/frame_host/interstitial_page_impl.cc:840:
> rvh->GetDelegate()->GetFrameTree(), frame_callback);
> Suggestion: IMO, this code would be simpler and more understandable if it just
> called NotifyForEachFrame with the constructed callback from each branch of
the
> switch statement.
> 
>
https://codereview.chromium.org/1542743002/diff/260001/content/browser/loader...
> File content/browser/loader/resource_dispatcher_host_impl.cc (left):
> 
>
https://codereview.chromium.org/1542743002/diff/260001/content/browser/loader...
> content/browser/loader/resource_dispatcher_host_impl.cc:1744:
> scheduler_->OnClientDeleted(child_id, route_id);
> I thought this was going to be removed; planning to yank it in a different CL?
> 
>
https://codereview.chromium.org/1542743002/diff/260001/content/browser/loader...
> File content/browser/loader/resource_dispatcher_host_impl.cc (right):
> 
>
https://codereview.chromium.org/1542743002/diff/260001/content/browser/loader...
> content/browser/loader/resource_dispatcher_host_impl.cc:461: FrameTreeNode*
> tree_node) {
> It looks to me like tree_node is purely an input argument, so it could be
const
> (even a const&), and I think by the style guide it should go before the output
> arguments.
> 
>
https://codereview.chromium.org/1542743002/diff/260001/content/browser/loader...
> File content/browser/loader/resource_dispatcher_host_impl.h (right):
> 
>
https://codereview.chromium.org/1542743002/diff/260001/content/browser/loader...
> content/browser/loader/resource_dispatcher_host_impl.h:41: #include
> "content/public/browser/render_view_host.h"
> Why?
> 
>
https://codereview.chromium.org/1542743002/diff/260001/content/browser/web_co...
> File content/browser/web_contents/web_contents_impl.cc (right):
> 
>
https://codereview.chromium.org/1542743002/diff/260001/content/browser/web_co...
> content/browser/web_contents/web_contents_impl.cc:1842: // source.
> The second sentence "feels" to me as if it's an implementation dependency of
an
> implementation well removed from this code.  Am I intuiting correctly?  If so,
I
> think it's an unwise thing to rely on (and hence shouldn't be in the comment).

Thank you both for the reviews! Randy, let's meet tomorrow to flesh this out in
a final pass.

One thing: I would really like to iron out the API so that UI methods take UI
objects, not routes. This is stymied by WebContentsImpl::CreateNewWindow, which
doesn't have access to the frame at that point, the creation of the WebContents
has been taken by the delegate.

I need to investigate whether we can tell the delegate to do this, or how to
extract the frame from the delegate. This is further complicated by the fact
that often the delegate won't create a new WebContents.

Charlie Harrison

On 2016/01/21 00:56:06, csharrison wrote: > On 2016/01/20 23:23:19, Randy Smith - Not in Fridays ...

4 years, 11 months ago (2016-01-21 14:47:30 UTC) #18

On 2016/01/21 00:56:06, csharrison wrote:
> On 2016/01/20 23:23:19, Randy Smith - Not in Fridays wrote:
> > I'm still chewing on the architecture of RDH/LIOTN.  On the bright side,
once
> we
> > nail that down, I doubt I'll have much more to say on this CL :-}.
> > 
> > Preface for context: I have the sense that you're making the choice to have
> all
> > the UI thread interfaces be static and all of the non-UI thread interfaces
be
> > regular methods.  In the abstract I think that's a good choice--it makes it
> very
> > clear what thread the data members live on, while keeping related
> functionality
> > together on the class.  But it has some implications that I've poked at in
the
> > following.  
> > 
> > * Given the static<->UI thread choice, I think that the NotifyFor*
interfaces
> > are a bit of a cheat, since they actually target/require knowledge of IO
> thread
> > interfaces on the UI thread, even though those interfaces aren't actually
> called
> > on the UI thread.  I'm of the opinion that the overall interface would be
> > cleaner if we instead had UI thread interfaces {Block,
> > {Resume,Cancel}Blocked}RequestsForFrameTree.  I think the set of interfaces
> that
> > would result in having on RDHI would be more coherent and easier for new
users
> > of the class to grok; the Notify* interfaces force people into using
function
> > pointers (and class method function pointers, which are even a bit more
> > confusing), but don't really provide a lot of extra power for that--there
are
> > only a couple of options.  Let's just give the consumer those options. 
> > 
> > As an API simplification (up to you), I could imagine those interfaces being
> > "ForFrame" with a boolean indicating whether recursion from that frame was
> > desired, or we could simply make them always recurse (presuming you can get
> the
> > descendants of a frame from a frame, and presuming that the times when we're
> > targeting a single frame that frame is known to have no children) so that we
> > don't need separate interfaces/boolean argument to distinguish ForFrame and
> > ForFrameTree.
> > 
> > * I'd suggest that if we're going to make the UI/IO "appropriate API"
> division,
> > we restrict ourselves to using UI objects on the UI thread and routes on the
> IO
> > thread just for simplification of the API.  So this would mean that
> > WebContentsImpl::CreateNewWindow would call ResumeBlockedRequestsForFrame
(not
> > *Route).  LoaderIOThreadNotifer doesn't count; it's effectively an appendage
> > class of ResourceDispatcherHostImpl.
> > 
> > * (Suggestion) At this point, LoaderIOThreadNotifier isn't really doing
> > much--it's just forwarding UI thread notifications to UI thread interfaces
on
> > the RDHI.  I think I'd find the overall interface cleaner if that class was
a
> > private class for RDHI (declared and defined in the .cc file) and there was
a
> > (static) method on RDHI that was called when a WebContents was created that
> > attached a class instance to the contents.  That keeps the new header
> > declarations visible to a minimum.
> > 
> > (I wanted to find a way to make RDHI a WCO, but it's gotta observer a *lot*
of
> > WCOs, and the WCO interface isn't built for that.  Which I think is a flaw
in
> > the interface, but hey, what so I know :-}, and I'm certainly not going to
ask
> > you to fix it.)
> > 
> > * The UI Thread->static choice also results in the pattern of declaring
static
> > methods on the RDH abstract base class that are functionally equivalent to
how
> > interfaces on abstract base classes are used (to delegate to the derived
> class).
> >  In other words, it's *almost* something that people are very familiar with
> and
> > used to, but not *quite*.  And the implementation only works because of the
> use
> > of the global accessor (RDHI::Get()).  I dislike RDHI::Get(), but it's used
> all
> > over the place, and it'd be real mission creep for this CL to push you to
fix
> > it, so I won't :-}.  Having said that, I don't like perpetuating it, and
> writing
> > an interface that requires it feels like a perpetuation to me.  The
> combination
> > of those two discomforts makes me want to push you to make these non-static
> pure
> > virtual methods that are implemented in RDHI.  But that violates the
> > architectural invariant I think you're trying to setup, so I'm torn--I see
why
> > you're doing what you're doing, but every time I look at that pattern I
wince.
> 
> > We may want to talk about this in person.  (As an alternative path, if Matt
is
> > neutral to positive about your pattern, I would yield to his arch sense.)
> > 
> >
>
https://codereview.chromium.org/1542743002/diff/260001/content/browser/frame_...
> > File content/browser/frame_host/interstitial_page_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1542743002/diff/260001/content/browser/frame_...
> > content/browser/frame_host/interstitial_page_impl.cc:821: // have one.
> > I think this comment isn't needed anymore.
> > 
> >
>
https://codereview.chromium.org/1542743002/diff/260001/content/browser/frame_...
> > content/browser/frame_host/interstitial_page_impl.cc:840:
> > rvh->GetDelegate()->GetFrameTree(), frame_callback);
> > Suggestion: IMO, this code would be simpler and more understandable if it
just
> > called NotifyForEachFrame with the constructed callback from each branch of
> the
> > switch statement.
> > 
> >
>
https://codereview.chromium.org/1542743002/diff/260001/content/browser/loader...
> > File content/browser/loader/resource_dispatcher_host_impl.cc (left):
> > 
> >
>
https://codereview.chromium.org/1542743002/diff/260001/content/browser/loader...
> > content/browser/loader/resource_dispatcher_host_impl.cc:1744:
> > scheduler_->OnClientDeleted(child_id, route_id);
> > I thought this was going to be removed; planning to yank it in a different
CL?
> > 
> >
>
https://codereview.chromium.org/1542743002/diff/260001/content/browser/loader...
> > File content/browser/loader/resource_dispatcher_host_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1542743002/diff/260001/content/browser/loader...
> > content/browser/loader/resource_dispatcher_host_impl.cc:461: FrameTreeNode*
> > tree_node) {
> > It looks to me like tree_node is purely an input argument, so it could be
> const
> > (even a const&), and I think by the style guide it should go before the
output
> > arguments.
> > 
> >
>
https://codereview.chromium.org/1542743002/diff/260001/content/browser/loader...
> > File content/browser/loader/resource_dispatcher_host_impl.h (right):
> > 
> >
>
https://codereview.chromium.org/1542743002/diff/260001/content/browser/loader...
> > content/browser/loader/resource_dispatcher_host_impl.h:41: #include
> > "content/public/browser/render_view_host.h"
> > Why?
> > 
> >
>
https://codereview.chromium.org/1542743002/diff/260001/content/browser/web_co...
> > File content/browser/web_contents/web_contents_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1542743002/diff/260001/content/browser/web_co...
> > content/browser/web_contents/web_contents_impl.cc:1842: // source.
> > The second sentence "feels" to me as if it's an implementation dependency of
> an
> > implementation well removed from this code.  Am I intuiting correctly?  If
so,
> I
> > think it's an unwise thing to rely on (and hence shouldn't be in the
comment).
> 
> Thank you both for the reviews! Randy, let's meet tomorrow to flesh this out
in
> a final pass.
> 
> One thing: I would really like to iron out the API so that UI methods take UI
> objects, not routes. This is stymied by WebContentsImpl::CreateNewWindow,
which
> doesn't have access to the frame at that point, the creation of the
WebContents
> has been taken by the delegate.
> 
> I need to investigate whether we can tell the delegate to do this, or how to
> extract the frame from the delegate. This is further complicated by the fact
> that often the delegate won't create a new WebContents.

After thinking about it a little more, the code snippet in WebContentsImpl can't
use a frame, because a common use case is
1. Block requests for route id (on io thread)
2. Delegate decides to block the new window (not create a WebContents)
3. We never have a frame host in the browser process (similar to the snippet
above it that fails to find a RenderViewHost)

For this reason, I think the better API isn't feasible. I'm still open to class
methods over static methods though to avoid a public RDH impl, I just want to be
sure we aren't being racy,

Charlie Harrison

Responded to comments without making architectural changes. Hopefully will decide w/ Randy tomorrow how to ...

4 years, 11 months ago (2016-01-21 18:52:56 UTC) #19

Randy Smith (Not in Mondays)

On 2016/01/21 18:52:56, csharrison wrote: > Responded to comments without making architectural changes. Hopefully will ...

4 years, 11 months ago (2016-01-22 17:38:59 UTC) #20

Charlie Harrison

On 2016/01/22 17:38:59, Randy Smith - Not in Fridays wrote: > On 2016/01/21 18:52:56, csharrison ...

4 years, 11 months ago (2016-01-25 16:13:47 UTC) #21

On 2016/01/22 17:38:59, Randy Smith - Not in Fridays wrote:
> On 2016/01/21 18:52:56, csharrison wrote:
> > Responded to comments without making architectural changes. Hopefully will
> > decide w/ Randy tomorrow how to structure this and shoot up another CL by
the
> > weekend. We'll see.
> 
> Just to make sure we're on the same page based on our conversation offline;
I'm
> withdrawing my comments about the RDH static methods, but the rest of my
> architectural comments/suggestions stand, and you don't need another review
from
> me until you've responded to those.  Let me know if any of that doesn't match
> your perspective.

Yep, on the same page. This is the best revamped API I can come up with:

1. Remove NotifyFor/NotifyForEach. This entails creating Cancel/Block/Resume for
Frame / Route methods.

2. Add a boolean to Cancel/Block/ResumeForFrame method indicating that the
method should work on the entire frame tree. This is a little kludgey, but we
can assert that the node passed in is the main frame node and call it something
like "should_recur_on_main_frame."

This gives us the following new API

content/public
BlockRequestsForFrame(framehost, recur)
ResumeRequestsForFrame(framehost, recur)

content/
CancelBlockedRequestsForFrame(framehost, recur)
BlockRequestsForRoute(route)
ResumeBlockedRequestsForRoute(route)

The ForRoute methods will be used by the WebContentsImpl NewWindow code path
only (no framehost available on UI thread, other call is on IO thread).

WDYT? I'll start implementing when you give signoff. The other problem with this
API is that it gives content/public access to the FrameTree API, so we could
also do the following:
content/public
BlockRequestsForFrame(framehost)
ResumeRequestsForFrame(framehost)

content/
BlockRequestsForFrame(framehost, recur)
ResumeRequestsForFrame(framehost, recur)
CancelBlockedRequestsForFrame(framehost, recur)
BlockRequestsForRoute(route)
ResumeBlockedRequestsForRoute(route)

But that's worse than two APIs that take FrameHosts and FrameTrees,
respectively.

Randy Smith (Not in Mondays)

On 2016/01/25 16:13:47, csharrison wrote: > On 2016/01/22 17:38:59, Randy Smith - Not in Fridays ...

4 years, 11 months ago (2016-01-26 20:32:12 UTC) #22

On 2016/01/25 16:13:47, csharrison wrote:
> On 2016/01/22 17:38:59, Randy Smith - Not in Fridays wrote:
> > On 2016/01/21 18:52:56, csharrison wrote:
> > > Responded to comments without making architectural changes. Hopefully will
> > > decide w/ Randy tomorrow how to structure this and shoot up another CL by
> the
> > > weekend. We'll see.
> > 
> > Just to make sure we're on the same page based on our conversation offline;
> I'm
> > withdrawing my comments about the RDH static methods, but the rest of my
> > architectural comments/suggestions stand, and you don't need another review
> from
> > me until you've responded to those.  Let me know if any of that doesn't
match
> > your perspective.
> 
> Yep, on the same page. This is the best revamped API I can come up with:
> 
> 1. Remove NotifyFor/NotifyForEach. This entails creating Cancel/Block/Resume
for
> Frame / Route methods.
> 
> 2. Add a boolean to Cancel/Block/ResumeForFrame method indicating that the
> method should work on the entire frame tree. This is a little kludgey, but we
> can assert that the node passed in is the main frame node and call it
something
> like "should_recur_on_main_frame."
> 
> 
> This gives us the following new API
> 
> content/public
> BlockRequestsForFrame(framehost, recur)
> ResumeRequestsForFrame(framehost, recur)
> 
> content/
> CancelBlockedRequestsForFrame(framehost, recur)
> BlockRequestsForRoute(route)
> ResumeBlockedRequestsForRoute(route)
> 
> The ForRoute methods will be used by the WebContentsImpl NewWindow code path
> only (no framehost available on UI thread, other call is on IO thread).
> 
> WDYT? I'll start implementing when you give signoff. The other problem with
this
> API is that it gives content/public access to the FrameTree API, so we could
> also do the following:

So I like the above API quite a bit; thank you.  Having said that, unless the
API's going to be much worse without, I'd lean against adding stuff to the
content/ interface (== content/public), and you should certainly run the change
by a content/ OWNER before implementing, as that's exactly the area that they
tend to be most conservative around.  Having said *that*, I don't see why you
need to add the FrameTree concept to the interface?  It's just adding a boolean
to the interface you sketch out below.  (If what you mean is giving the
*implementation* of methods in content/public access to the FrameTree API, I
think that's ok, and also avoidable if it's not (just indirect directly into the
same static on the RDHI) but you should probably check with a content/ OWNER
just in case).

> content/public
> BlockRequestsForFrame(framehost)
> ResumeRequestsForFrame(framehost)
> 
> content/
> BlockRequestsForFrame(framehost, recur)
> ResumeRequestsForFrame(framehost, recur)

I think having the same name but different signatures on static methods of two
classes that have an inheritance relationship is an awesome recipe for confusion
and mistakes; I'd recommend against :-}.

> CancelBlockedRequestsForFrame(framehost, recur)
> BlockRequestsForRoute(route)
> ResumeBlockedRequestsForRoute(route)
> 
> But that's worse than two APIs that take FrameHosts and FrameTrees,
> respectively.

Charlie Harrison

On 2016/01/26 20:32:12, Randy Smith - Not in Fridays wrote: > On 2016/01/25 16:13:47, csharrison ...

4 years, 11 months ago (2016-01-26 20:48:10 UTC) #23

On 2016/01/26 20:32:12, Randy Smith - Not in Fridays wrote:
> On 2016/01/25 16:13:47, csharrison wrote:
> > On 2016/01/22 17:38:59, Randy Smith - Not in Fridays wrote:
> > > On 2016/01/21 18:52:56, csharrison wrote:
> > > > Responded to comments without making architectural changes. Hopefully
will
> > > > decide w/ Randy tomorrow how to structure this and shoot up another CL
by
> > the
> > > > weekend. We'll see.
> > > 
> > > Just to make sure we're on the same page based on our conversation
offline;
> > I'm
> > > withdrawing my comments about the RDH static methods, but the rest of my
> > > architectural comments/suggestions stand, and you don't need another
review
> > from
> > > me until you've responded to those.  Let me know if any of that doesn't
> match
> > > your perspective.
> > 
> > Yep, on the same page. This is the best revamped API I can come up with:
> > 
> > 1. Remove NotifyFor/NotifyForEach. This entails creating Cancel/Block/Resume
> for
> > Frame / Route methods.
> > 
> > 2. Add a boolean to Cancel/Block/ResumeForFrame method indicating that the
> > method should work on the entire frame tree. This is a little kludgey, but
we
> > can assert that the node passed in is the main frame node and call it
> something
> > like "should_recur_on_main_frame."
> > 
> > 
> > This gives us the following new API
> > 
> > content/public
> > BlockRequestsForFrame(framehost, recur)
> > ResumeRequestsForFrame(framehost, recur)
> > 
> > content/
> > CancelBlockedRequestsForFrame(framehost, recur)
> > BlockRequestsForRoute(route)
> > ResumeBlockedRequestsForRoute(route)
> > 
> > The ForRoute methods will be used by the WebContentsImpl NewWindow code path
> > only (no framehost available on UI thread, other call is on IO thread).
> > 
> > WDYT? I'll start implementing when you give signoff. The other problem with
> this
> > API is that it gives content/public access to the FrameTree API, so we could
> > also do the following:
> 
> So I like the above API quite a bit; thank you.  Having said that, unless the
> API's going to be much worse without, I'd lean against adding stuff to the
> content/ interface (== content/public), and you should certainly run the
change
> by a content/ OWNER before implementing, as that's exactly the area that they
> tend to be most conservative around.  Having said *that*, I don't see why you
> need to add the FrameTree concept to the interface?  It's just adding a
boolean
> to the interface you sketch out below.  (If what you mean is giving the
> *implementation* of methods in content/public access to the FrameTree API, I
> think that's ok, and also avoidable if it's not (just indirect directly into
the
> same static on the RDHI) but you should probably check with a content/ OWNER
> just in case).
> 
> > content/public
> > BlockRequestsForFrame(framehost)
> > ResumeRequestsForFrame(framehost)
> > 
> > content/
> > BlockRequestsForFrame(framehost, recur)
> > ResumeRequestsForFrame(framehost, recur)
> 
> I think having the same name but different signatures on static methods of two
> classes that have an inheritance relationship is an awesome recipe for
confusion
> and mistakes; I'd recommend against :-}.
> 
> > CancelBlockedRequestsForFrame(framehost, recur)
> > BlockRequestsForRoute(route)
> > ResumeBlockedRequestsForRoute(route)
> > 
> > But that's worse than two APIs that take FrameHosts and FrameTrees,
> > respectively.

Yeah it would be the implementation (i.e. they just take a bool). Consumers
won't need access to FrameTrees. If that's too much I guess the next best thing
is
content/public
BlockRequestsForFrame(framehost)
ResumeRequestsForFrame(framehost)

content/
BlockRequestsForFrameTree(tree)
ResumeRequestsForFrameTree(tree)
CancelBlockedRequestsForFrameTree(tree)
BlockRequestsForRoute(route)
ResumeBlockedRequestsForRoute(route)

Randy Smith (Not in Mondays)

On 2016/01/26 20:48:10, csharrison wrote: > On 2016/01/26 20:32:12, Randy Smith - Not in Fridays ...

4 years, 11 months ago (2016-01-26 20:49:51 UTC) #24

On 2016/01/26 20:48:10, csharrison wrote:
> On 2016/01/26 20:32:12, Randy Smith - Not in Fridays wrote:
> > On 2016/01/25 16:13:47, csharrison wrote:
> > > On 2016/01/22 17:38:59, Randy Smith - Not in Fridays wrote:
> > > > On 2016/01/21 18:52:56, csharrison wrote:
> > > > > Responded to comments without making architectural changes. Hopefully
> will
> > > > > decide w/ Randy tomorrow how to structure this and shoot up another CL
> by
> > > the
> > > > > weekend. We'll see.
> > > > 
> > > > Just to make sure we're on the same page based on our conversation
> offline;
> > > I'm
> > > > withdrawing my comments about the RDH static methods, but the rest of my
> > > > architectural comments/suggestions stand, and you don't need another
> review
> > > from
> > > > me until you've responded to those.  Let me know if any of that doesn't
> > match
> > > > your perspective.
> > > 
> > > Yep, on the same page. This is the best revamped API I can come up with:
> > > 
> > > 1. Remove NotifyFor/NotifyForEach. This entails creating
Cancel/Block/Resume
> > for
> > > Frame / Route methods.
> > > 
> > > 2. Add a boolean to Cancel/Block/ResumeForFrame method indicating that the
> > > method should work on the entire frame tree. This is a little kludgey, but
> we
> > > can assert that the node passed in is the main frame node and call it
> > something
> > > like "should_recur_on_main_frame."
> > > 
> > > 
> > > This gives us the following new API
> > > 
> > > content/public
> > > BlockRequestsForFrame(framehost, recur)
> > > ResumeRequestsForFrame(framehost, recur)
> > > 
> > > content/
> > > CancelBlockedRequestsForFrame(framehost, recur)
> > > BlockRequestsForRoute(route)
> > > ResumeBlockedRequestsForRoute(route)
> > > 
> > > The ForRoute methods will be used by the WebContentsImpl NewWindow code
path
> > > only (no framehost available on UI thread, other call is on IO thread).
> > > 
> > > WDYT? I'll start implementing when you give signoff. The other problem
with
> > this
> > > API is that it gives content/public access to the FrameTree API, so we
could
> > > also do the following:
> > 
> > So I like the above API quite a bit; thank you.  Having said that, unless
the
> > API's going to be much worse without, I'd lean against adding stuff to the
> > content/ interface (== content/public), and you should certainly run the
> change
> > by a content/ OWNER before implementing, as that's exactly the area that
they
> > tend to be most conservative around.  Having said *that*, I don't see why
you
> > need to add the FrameTree concept to the interface?  It's just adding a
> boolean
> > to the interface you sketch out below.  (If what you mean is giving the
> > *implementation* of methods in content/public access to the FrameTree API, I
> > think that's ok, and also avoidable if it's not (just indirect directly into
> the
> > same static on the RDHI) but you should probably check with a content/ OWNER
> > just in case).
> > 
> > > content/public
> > > BlockRequestsForFrame(framehost)
> > > ResumeRequestsForFrame(framehost)
> > > 
> > > content/
> > > BlockRequestsForFrame(framehost, recur)
> > > ResumeRequestsForFrame(framehost, recur)
> > 
> > I think having the same name but different signatures on static methods of
two
> > classes that have an inheritance relationship is an awesome recipe for
> confusion
> > and mistakes; I'd recommend against :-}.
> > 
> > > CancelBlockedRequestsForFrame(framehost, recur)
> > > BlockRequestsForRoute(route)
> > > ResumeBlockedRequestsForRoute(route)
> > > 
> > > But that's worse than two APIs that take FrameHosts and FrameTrees,
> > > respectively.
> 
> Yeah it would be the implementation (i.e. they just take a bool). Consumers
> won't need access to FrameTrees. If that's too much I guess the next best
thing
> is

I think an implementation dependency is fine, actually.  So +1 for your first
API plan.

> content/public
> BlockRequestsForFrame(framehost)
> ResumeRequestsForFrame(framehost)
> 
> content/
> BlockRequestsForFrameTree(tree)
> ResumeRequestsForFrameTree(tree)
> CancelBlockedRequestsForFrameTree(tree)
> BlockRequestsForRoute(route)
> ResumeBlockedRequestsForRoute(route)

nasko

On 2016/01/26 20:49:51, Randy Smith - Not in Fridays wrote: > On 2016/01/26 20:48:10, csharrison ...

4 years, 11 months ago (2016-01-27 18:31:39 UTC) #25

On 2016/01/26 20:49:51, Randy Smith - Not in Fridays wrote:
> On 2016/01/26 20:48:10, csharrison wrote:
> > On 2016/01/26 20:32:12, Randy Smith - Not in Fridays wrote:
> > > On 2016/01/25 16:13:47, csharrison wrote:
> > > > On 2016/01/22 17:38:59, Randy Smith - Not in Fridays wrote:
> > > > > On 2016/01/21 18:52:56, csharrison wrote:
> > > > > > Responded to comments without making architectural changes.
Hopefully
> > will
> > > > > > decide w/ Randy tomorrow how to structure this and shoot up another
CL
> > by
> > > > the
> > > > > > weekend. We'll see.
> > > > > 
> > > > > Just to make sure we're on the same page based on our conversation
> > offline;
> > > > I'm
> > > > > withdrawing my comments about the RDH static methods, but the rest of
my
> > > > > architectural comments/suggestions stand, and you don't need another
> > review
> > > > from
> > > > > me until you've responded to those.  Let me know if any of that
doesn't
> > > match
> > > > > your perspective.
> > > > 
> > > > Yep, on the same page. This is the best revamped API I can come up with:
> > > > 
> > > > 1. Remove NotifyFor/NotifyForEach. This entails creating
> Cancel/Block/Resume
> > > for
> > > > Frame / Route methods.
> > > > 
> > > > 2. Add a boolean to Cancel/Block/ResumeForFrame method indicating that
the
> > > > method should work on the entire frame tree. This is a little kludgey,
but
> > we
> > > > can assert that the node passed in is the main frame node and call it
> > > something
> > > > like "should_recur_on_main_frame."
> > > > 
> > > > 
> > > > This gives us the following new API
> > > > 
> > > > content/public
> > > > BlockRequestsForFrame(framehost, recur)
> > > > ResumeRequestsForFrame(framehost, recur)
> > > > 
> > > > content/
> > > > CancelBlockedRequestsForFrame(framehost, recur)
> > > > BlockRequestsForRoute(route)
> > > > ResumeBlockedRequestsForRoute(route)
> > > > 
> > > > The ForRoute methods will be used by the WebContentsImpl NewWindow code
> path
> > > > only (no framehost available on UI thread, other call is on IO thread).
> > > > 
> > > > WDYT? I'll start implementing when you give signoff. The other problem
> with
> > > this
> > > > API is that it gives content/public access to the FrameTree API, so we
> could
> > > > also do the following:
> > > 
> > > So I like the above API quite a bit; thank you.  Having said that, unless
> the
> > > API's going to be much worse without, I'd lean against adding stuff to the
> > > content/ interface (== content/public), and you should certainly run the
> > change
> > > by a content/ OWNER before implementing, as that's exactly the area that
> they
> > > tend to be most conservative around.  Having said *that*, I don't see why
> you
> > > need to add the FrameTree concept to the interface?  It's just adding a
> > boolean
> > > to the interface you sketch out below.  (If what you mean is giving the
> > > *implementation* of methods in content/public access to the FrameTree API,
I
> > > think that's ok, and also avoidable if it's not (just indirect directly
into
> > the
> > > same static on the RDHI) but you should probably check with a content/
OWNER
> > > just in case).
> > > 
> > > > content/public
> > > > BlockRequestsForFrame(framehost)
> > > > ResumeRequestsForFrame(framehost)
> > > > 
> > > > content/
> > > > BlockRequestsForFrame(framehost, recur)
> > > > ResumeRequestsForFrame(framehost, recur)
> > > 
> > > I think having the same name but different signatures on static methods of
> two
> > > classes that have an inheritance relationship is an awesome recipe for
> > confusion
> > > and mistakes; I'd recommend against :-}.
> > > 
> > > > CancelBlockedRequestsForFrame(framehost, recur)
> > > > BlockRequestsForRoute(route)
> > > > ResumeBlockedRequestsForRoute(route)
> > > > 
> > > > But that's worse than two APIs that take FrameHosts and FrameTrees,
> > > > respectively.
> > 
> > Yeah it would be the implementation (i.e. they just take a bool). Consumers
> > won't need access to FrameTrees. If that's too much I guess the next best
> thing
> > is
> 
> I think an implementation dependency is fine, actually.  So +1 for your first
> API plan.

Just to be explicit, can you list which is the last standing proposal?

In general, we don't want to expose FrameTree outside of content/.
 
> > content/public
> > BlockRequestsForFrame(framehost)
> > ResumeRequestsForFrame(framehost)
> > 
> > content/
> > BlockRequestsForFrameTree(tree)
> > ResumeRequestsForFrameTree(tree)
> > CancelBlockedRequestsForFrameTree(tree)
> > BlockRequestsForRoute(route)
> > ResumeBlockedRequestsForRoute(route)

Randy Smith (Not in Mondays)

On 2016/01/27 18:31:39, nasko wrote: > On 2016/01/26 20:49:51, Randy Smith - Not in Fridays ...

4 years, 11 months ago (2016-01-27 19:13:57 UTC) #26

On 2016/01/27 18:31:39, nasko wrote:
> On 2016/01/26 20:49:51, Randy Smith - Not in Fridays wrote:
> > On 2016/01/26 20:48:10, csharrison wrote:
> > > On 2016/01/26 20:32:12, Randy Smith - Not in Fridays wrote:
> > > > On 2016/01/25 16:13:47, csharrison wrote:
> > > > > On 2016/01/22 17:38:59, Randy Smith - Not in Fridays wrote:
> > > > > > On 2016/01/21 18:52:56, csharrison wrote:
> > > > > > > Responded to comments without making architectural changes.
> Hopefully
> > > will
> > > > > > > decide w/ Randy tomorrow how to structure this and shoot up
another
> CL
> > > by
> > > > > the
> > > > > > > weekend. We'll see.
> > > > > > 
> > > > > > Just to make sure we're on the same page based on our conversation
> > > offline;
> > > > > I'm
> > > > > > withdrawing my comments about the RDH static methods, but the rest
of
> my
> > > > > > architectural comments/suggestions stand, and you don't need another
> > > review
> > > > > from
> > > > > > me until you've responded to those.  Let me know if any of that
> doesn't
> > > > match
> > > > > > your perspective.
> > > > > 
> > > > > Yep, on the same page. This is the best revamped API I can come up
with:
> > > > > 
> > > > > 1. Remove NotifyFor/NotifyForEach. This entails creating
> > Cancel/Block/Resume
> > > > for
> > > > > Frame / Route methods.
> > > > > 
> > > > > 2. Add a boolean to Cancel/Block/ResumeForFrame method indicating that
> the
> > > > > method should work on the entire frame tree. This is a little kludgey,
> but
> > > we
> > > > > can assert that the node passed in is the main frame node and call it
> > > > something
> > > > > like "should_recur_on_main_frame."
> > > > > 
> > > > > 
> > > > > This gives us the following new API
> > > > > 
> > > > > content/public
> > > > > BlockRequestsForFrame(framehost, recur)
> > > > > ResumeRequestsForFrame(framehost, recur)
> > > > > 
> > > > > content/
> > > > > CancelBlockedRequestsForFrame(framehost, recur)
> > > > > BlockRequestsForRoute(route)
> > > > > ResumeBlockedRequestsForRoute(route)
> > > > > 
> > > > > The ForRoute methods will be used by the WebContentsImpl NewWindow
code
> > path
> > > > > only (no framehost available on UI thread, other call is on IO
thread).
> > > > > 
> > > > > WDYT? I'll start implementing when you give signoff. The other problem
> > with
> > > > this
> > > > > API is that it gives content/public access to the FrameTree API, so we
> > could
> > > > > also do the following:
> > > > 
> > > > So I like the above API quite a bit; thank you.  Having said that,
unless
> > the
> > > > API's going to be much worse without, I'd lean against adding stuff to
the
> > > > content/ interface (== content/public), and you should certainly run the
> > > change
> > > > by a content/ OWNER before implementing, as that's exactly the area that
> > they
> > > > tend to be most conservative around.  Having said *that*, I don't see
why
> > you
> > > > need to add the FrameTree concept to the interface?  It's just adding a
> > > boolean
> > > > to the interface you sketch out below.  (If what you mean is giving the
> > > > *implementation* of methods in content/public access to the FrameTree
API,
> I
> > > > think that's ok, and also avoidable if it's not (just indirect directly
> into
> > > the
> > > > same static on the RDHI) but you should probably check with a content/
> OWNER
> > > > just in case).
> > > > 
> > > > > content/public
> > > > > BlockRequestsForFrame(framehost)
> > > > > ResumeRequestsForFrame(framehost)
> > > > > 
> > > > > content/
> > > > > BlockRequestsForFrame(framehost, recur)
> > > > > ResumeRequestsForFrame(framehost, recur)
> > > > 
> > > > I think having the same name but different signatures on static methods
of
> > two
> > > > classes that have an inheritance relationship is an awesome recipe for
> > > confusion
> > > > and mistakes; I'd recommend against :-}.
> > > > 
> > > > > CancelBlockedRequestsForFrame(framehost, recur)
> > > > > BlockRequestsForRoute(route)
> > > > > ResumeBlockedRequestsForRoute(route)
> > > > > 
> > > > > But that's worse than two APIs that take FrameHosts and FrameTrees,
> > > > > respectively.
> > > 
> > > Yeah it would be the implementation (i.e. they just take a bool).
Consumers
> > > won't need access to FrameTrees. If that's too much I guess the next best
> > thing
> > > is
> > 
> > I think an implementation dependency is fine, actually.  So +1 for your
first
> > API plan.
> 
> Just to be explicit, can you list which is the last standing proposal?

I am in favor of the first API described in c#21.

> In general, we don't want to expose FrameTree outside of content/.

This exposes the tuple (RenderFrameHost, bool) outside of content/, which I
presume is ok.  It's conceptually exposing the frame tree (the bool indicates
whether you'll recurse down it or not) but not exposing that type.  I presume
that's ok, since I presume the concept of frames having frames in them is sorta
obvious, but I wanted a content/ OWNER's opinion (i.e. your opinion) on it as
well.

-- Randy

>  
> > > content/public
> > > BlockRequestsForFrame(framehost)
> > > ResumeRequestsForFrame(framehost)
> > > 
> > > content/
> > > BlockRequestsForFrameTree(tree)
> > > ResumeRequestsForFrameTree(tree)
> > > CancelBlockedRequestsForFrameTree(tree)
> > > BlockRequestsForRoute(route)
> > > ResumeBlockedRequestsForRoute(route)

nasko

On 2016/01/27 19:13:57, Randy Smith - Not in Fridays wrote: > On 2016/01/27 18:31:39, nasko ...

4 years, 11 months ago (2016-01-27 19:28:37 UTC) #27

On 2016/01/27 19:13:57, Randy Smith - Not in Fridays wrote:
> On 2016/01/27 18:31:39, nasko wrote:
> > On 2016/01/26 20:49:51, Randy Smith - Not in Fridays wrote:
> > > On 2016/01/26 20:48:10, csharrison wrote:
> > > > On 2016/01/26 20:32:12, Randy Smith - Not in Fridays wrote:
> > > > > On 2016/01/25 16:13:47, csharrison wrote:
> > > > > > On 2016/01/22 17:38:59, Randy Smith - Not in Fridays wrote:
> > > > > > > On 2016/01/21 18:52:56, csharrison wrote:
> > > > > > > > Responded to comments without making architectural changes.
> > Hopefully
> > > > will
> > > > > > > > decide w/ Randy tomorrow how to structure this and shoot up
> another
> > CL
> > > > by
> > > > > > the
> > > > > > > > weekend. We'll see.
> > > > > > > 
> > > > > > > Just to make sure we're on the same page based on our conversation
> > > > offline;
> > > > > > I'm
> > > > > > > withdrawing my comments about the RDH static methods, but the rest
> of
> > my
> > > > > > > architectural comments/suggestions stand, and you don't need
another
> > > > review
> > > > > > from
> > > > > > > me until you've responded to those.  Let me know if any of that
> > doesn't
> > > > > match
> > > > > > > your perspective.
> > > > > > 
> > > > > > Yep, on the same page. This is the best revamped API I can come up
> with:
> > > > > > 
> > > > > > 1. Remove NotifyFor/NotifyForEach. This entails creating
> > > Cancel/Block/Resume
> > > > > for
> > > > > > Frame / Route methods.
> > > > > > 
> > > > > > 2. Add a boolean to Cancel/Block/ResumeForFrame method indicating
that
> > the
> > > > > > method should work on the entire frame tree. This is a little
kludgey,
> > but
> > > > we
> > > > > > can assert that the node passed in is the main frame node and call
it
> > > > > something
> > > > > > like "should_recur_on_main_frame."
> > > > > > 
> > > > > > 
> > > > > > This gives us the following new API
> > > > > > 
> > > > > > content/public
> > > > > > BlockRequestsForFrame(framehost, recur)
> > > > > > ResumeRequestsForFrame(framehost, recur)
> > > > > > 
> > > > > > content/
> > > > > > CancelBlockedRequestsForFrame(framehost, recur)
> > > > > > BlockRequestsForRoute(route)
> > > > > > ResumeBlockedRequestsForRoute(route)
> > > > > > 
> > > > > > The ForRoute methods will be used by the WebContentsImpl NewWindow
> code
> > > path
> > > > > > only (no framehost available on UI thread, other call is on IO
> thread).
> > > > > > 
> > > > > > WDYT? I'll start implementing when you give signoff. The other
problem
> > > with
> > > > > this
> > > > > > API is that it gives content/public access to the FrameTree API, so
we
> > > could
> > > > > > also do the following:
> > > > > 
> > > > > So I like the above API quite a bit; thank you.  Having said that,
> unless
> > > the
> > > > > API's going to be much worse without, I'd lean against adding stuff to
> the
> > > > > content/ interface (== content/public), and you should certainly run
the
> > > > change
> > > > > by a content/ OWNER before implementing, as that's exactly the area
that
> > > they
> > > > > tend to be most conservative around.  Having said *that*, I don't see
> why
> > > you
> > > > > need to add the FrameTree concept to the interface?  It's just adding
a
> > > > boolean
> > > > > to the interface you sketch out below.  (If what you mean is giving
the
> > > > > *implementation* of methods in content/public access to the FrameTree
> API,
> > I
> > > > > think that's ok, and also avoidable if it's not (just indirect
directly
> > into
> > > > the
> > > > > same static on the RDHI) but you should probably check with a content/
> > OWNER
> > > > > just in case).
> > > > > 
> > > > > > content/public
> > > > > > BlockRequestsForFrame(framehost)
> > > > > > ResumeRequestsForFrame(framehost)
> > > > > > 
> > > > > > content/
> > > > > > BlockRequestsForFrame(framehost, recur)
> > > > > > ResumeRequestsForFrame(framehost, recur)
> > > > > 
> > > > > I think having the same name but different signatures on static
methods
> of
> > > two
> > > > > classes that have an inheritance relationship is an awesome recipe for
> > > > confusion
> > > > > and mistakes; I'd recommend against :-}.
> > > > > 
> > > > > > CancelBlockedRequestsForFrame(framehost, recur)
> > > > > > BlockRequestsForRoute(route)
> > > > > > ResumeBlockedRequestsForRoute(route)
> > > > > > 
> > > > > > But that's worse than two APIs that take FrameHosts and FrameTrees,
> > > > > > respectively.
> > > > 
> > > > Yeah it would be the implementation (i.e. they just take a bool).
> Consumers
> > > > won't need access to FrameTrees. If that's too much I guess the next
best
> > > thing
> > > > is
> > > 
> > > I think an implementation dependency is fine, actually.  So +1 for your
> first
> > > API plan.
> > 
> > Just to be explicit, can you list which is the last standing proposal?
> 
> I am in favor of the first API described in c#21.
> 
> > In general, we don't want to expose FrameTree outside of content/.
> 
> This exposes the tuple (RenderFrameHost, bool) outside of content/, which I
> presume is ok.  It's conceptually exposing the frame tree (the bool indicates
> whether you'll recurse down it or not) but not exposing that type.  I presume
> that's ok, since I presume the concept of frames having frames in them is
sorta
> obvious, but I wanted a content/ OWNER's opinion (i.e. your opinion) on it as
> well.

My only question is why do we need the recurse parameter? I have to look at all
the call sites for the new API, but in general I think we use it when doing
navigations, right? If that is the case, navigating any frame ends up destroying
all of its subframes, so no need to recurse into them. Also, we use it when
creating new WebContents, which by definition will only have one frame and will
not have subframes until we have unblocked the loads so a document can be
loaded.

> -- Randy
> 
> >  
> > > > content/public
> > > > BlockRequestsForFrame(framehost)
> > > > ResumeRequestsForFrame(framehost)
> > > > 
> > > > content/
> > > > BlockRequestsForFrameTree(tree)
> > > > ResumeRequestsForFrameTree(tree)
> > > > CancelBlockedRequestsForFrameTree(tree)
> > > > BlockRequestsForRoute(route)
> > > > ResumeBlockedRequestsForRoute(route)

Charlie Harrison

On 2016/01/27 19:28:37, nasko wrote: > On 2016/01/27 19:13:57, Randy Smith - Not in Fridays ...

4 years, 11 months ago (2016-01-27 19:31:18 UTC) #28

On 2016/01/27 19:28:37, nasko wrote:
> On 2016/01/27 19:13:57, Randy Smith - Not in Fridays wrote:
> > On 2016/01/27 18:31:39, nasko wrote:
> > > On 2016/01/26 20:49:51, Randy Smith - Not in Fridays wrote:
> > > > On 2016/01/26 20:48:10, csharrison wrote:
> > > > > On 2016/01/26 20:32:12, Randy Smith - Not in Fridays wrote:
> > > > > > On 2016/01/25 16:13:47, csharrison wrote:
> > > > > > > On 2016/01/22 17:38:59, Randy Smith - Not in Fridays wrote:
> > > > > > > > On 2016/01/21 18:52:56, csharrison wrote:
> > > > > > > > > Responded to comments without making architectural changes.
> > > Hopefully
> > > > > will
> > > > > > > > > decide w/ Randy tomorrow how to structure this and shoot up
> > another
> > > CL
> > > > > by
> > > > > > > the
> > > > > > > > > weekend. We'll see.
> > > > > > > > 
> > > > > > > > Just to make sure we're on the same page based on our
conversation
> > > > > offline;
> > > > > > > I'm
> > > > > > > > withdrawing my comments about the RDH static methods, but the
rest
> > of
> > > my
> > > > > > > > architectural comments/suggestions stand, and you don't need
> another
> > > > > review
> > > > > > > from
> > > > > > > > me until you've responded to those.  Let me know if any of that
> > > doesn't
> > > > > > match
> > > > > > > > your perspective.
> > > > > > > 
> > > > > > > Yep, on the same page. This is the best revamped API I can come up
> > with:
> > > > > > > 
> > > > > > > 1. Remove NotifyFor/NotifyForEach. This entails creating
> > > > Cancel/Block/Resume
> > > > > > for
> > > > > > > Frame / Route methods.
> > > > > > > 
> > > > > > > 2. Add a boolean to Cancel/Block/ResumeForFrame method indicating
> that
> > > the
> > > > > > > method should work on the entire frame tree. This is a little
> kludgey,
> > > but
> > > > > we
> > > > > > > can assert that the node passed in is the main frame node and call
> it
> > > > > > something
> > > > > > > like "should_recur_on_main_frame."
> > > > > > > 
> > > > > > > 
> > > > > > > This gives us the following new API
> > > > > > > 
> > > > > > > content/public
> > > > > > > BlockRequestsForFrame(framehost, recur)
> > > > > > > ResumeRequestsForFrame(framehost, recur)
> > > > > > > 
> > > > > > > content/
> > > > > > > CancelBlockedRequestsForFrame(framehost, recur)
> > > > > > > BlockRequestsForRoute(route)
> > > > > > > ResumeBlockedRequestsForRoute(route)
> > > > > > > 
> > > > > > > The ForRoute methods will be used by the WebContentsImpl NewWindow
> > code
> > > > path
> > > > > > > only (no framehost available on UI thread, other call is on IO
> > thread).
> > > > > > > 
> > > > > > > WDYT? I'll start implementing when you give signoff. The other
> problem
> > > > with
> > > > > > this
> > > > > > > API is that it gives content/public access to the FrameTree API,
so
> we
> > > > could
> > > > > > > also do the following:
> > > > > > 
> > > > > > So I like the above API quite a bit; thank you.  Having said that,
> > unless
> > > > the
> > > > > > API's going to be much worse without, I'd lean against adding stuff
to
> > the
> > > > > > content/ interface (== content/public), and you should certainly run
> the
> > > > > change
> > > > > > by a content/ OWNER before implementing, as that's exactly the area
> that
> > > > they
> > > > > > tend to be most conservative around.  Having said *that*, I don't
see
> > why
> > > > you
> > > > > > need to add the FrameTree concept to the interface?  It's just
adding
> a
> > > > > boolean
> > > > > > to the interface you sketch out below.  (If what you mean is giving
> the
> > > > > > *implementation* of methods in content/public access to the
FrameTree
> > API,
> > > I
> > > > > > think that's ok, and also avoidable if it's not (just indirect
> directly
> > > into
> > > > > the
> > > > > > same static on the RDHI) but you should probably check with a
content/
> > > OWNER
> > > > > > just in case).
> > > > > > 
> > > > > > > content/public
> > > > > > > BlockRequestsForFrame(framehost)
> > > > > > > ResumeRequestsForFrame(framehost)
> > > > > > > 
> > > > > > > content/
> > > > > > > BlockRequestsForFrame(framehost, recur)
> > > > > > > ResumeRequestsForFrame(framehost, recur)
> > > > > > 
> > > > > > I think having the same name but different signatures on static
> methods
> > of
> > > > two
> > > > > > classes that have an inheritance relationship is an awesome recipe
for
> > > > > confusion
> > > > > > and mistakes; I'd recommend against :-}.
> > > > > > 
> > > > > > > CancelBlockedRequestsForFrame(framehost, recur)
> > > > > > > BlockRequestsForRoute(route)
> > > > > > > ResumeBlockedRequestsForRoute(route)
> > > > > > > 
> > > > > > > But that's worse than two APIs that take FrameHosts and
FrameTrees,
> > > > > > > respectively.
> > > > > 
> > > > > Yeah it would be the implementation (i.e. they just take a bool).
> > Consumers
> > > > > won't need access to FrameTrees. If that's too much I guess the next
> best
> > > > thing
> > > > > is
> > > > 
> > > > I think an implementation dependency is fine, actually.  So +1 for your
> > first
> > > > API plan.
> > > 
> > > Just to be explicit, can you list which is the last standing proposal?
> > 
> > I am in favor of the first API described in c#21.
> > 
> > > In general, we don't want to expose FrameTree outside of content/.
> > 
> > This exposes the tuple (RenderFrameHost, bool) outside of content/, which I
> > presume is ok.  It's conceptually exposing the frame tree (the bool
indicates
> > whether you'll recurse down it or not) but not exposing that type.  I
presume
> > that's ok, since I presume the concept of frames having frames in them is
> sorta
> > obvious, but I wanted a content/ OWNER's opinion (i.e. your opinion) on it
as
> > well.
> 
> My only question is why do we need the recurse parameter? I have to look at
all
> the call sites for the new API, but in general I think we use it when doing
> navigations, right? If that is the case, navigating any frame ends up
destroying
> all of its subframes, so no need to recurse into them. Also, we use it when
> creating new WebContents, which by definition will only have one frame and
will
> not have subframes until we have unblocked the loads so a document can be
> loaded.
> 
> > -- Randy
> > 
> > >  
> > > > > content/public
> > > > > BlockRequestsForFrame(framehost)
> > > > > ResumeRequestsForFrame(framehost)
> > > > > 
> > > > > content/
> > > > > BlockRequestsForFrameTree(tree)
> > > > > ResumeRequestsForFrameTree(tree)
> > > > > CancelBlockedRequestsForFrameTree(tree)
> > > > > BlockRequestsForRoute(route)
> > > > > ResumeBlockedRequestsForRoute(route)

The only time we need it is for interstitials blocking their underlying page.
I'm fine with having a special (non content/public) interface for them if you
think that's a cleaner solution.

Randy Smith (Not in Mondays)

On 2016/01/27 19:31:18, csharrison wrote: > On 2016/01/27 19:28:37, nasko wrote: > > On 2016/01/27 ...

4 years, 11 months ago (2016-01-27 20:17:02 UTC) #29

On 2016/01/27 19:31:18, csharrison wrote:
> On 2016/01/27 19:28:37, nasko wrote:
> > On 2016/01/27 19:13:57, Randy Smith - Not in Fridays wrote:
> > > On 2016/01/27 18:31:39, nasko wrote:
> > > > On 2016/01/26 20:49:51, Randy Smith - Not in Fridays wrote:
> > > > > On 2016/01/26 20:48:10, csharrison wrote:
> > > > > > On 2016/01/26 20:32:12, Randy Smith - Not in Fridays wrote:
> > > > > > > On 2016/01/25 16:13:47, csharrison wrote:
> > > > > > > > On 2016/01/22 17:38:59, Randy Smith - Not in Fridays wrote:
> > > > > > > > > On 2016/01/21 18:52:56, csharrison wrote:
> > > > > > > > > > Responded to comments without making architectural changes.
> > > > Hopefully
> > > > > > will
> > > > > > > > > > decide w/ Randy tomorrow how to structure this and shoot up
> > > another
> > > > CL
> > > > > > by
> > > > > > > > the
> > > > > > > > > > weekend. We'll see.
> > > > > > > > > 
> > > > > > > > > Just to make sure we're on the same page based on our
> conversation
> > > > > > offline;
> > > > > > > > I'm
> > > > > > > > > withdrawing my comments about the RDH static methods, but the
> rest
> > > of
> > > > my
> > > > > > > > > architectural comments/suggestions stand, and you don't need
> > another
> > > > > > review
> > > > > > > > from
> > > > > > > > > me until you've responded to those.  Let me know if any of
that
> > > > doesn't
> > > > > > > match
> > > > > > > > > your perspective.
> > > > > > > > 
> > > > > > > > Yep, on the same page. This is the best revamped API I can come
up
> > > with:
> > > > > > > > 
> > > > > > > > 1. Remove NotifyFor/NotifyForEach. This entails creating
> > > > > Cancel/Block/Resume
> > > > > > > for
> > > > > > > > Frame / Route methods.
> > > > > > > > 
> > > > > > > > 2. Add a boolean to Cancel/Block/ResumeForFrame method
indicating
> > that
> > > > the
> > > > > > > > method should work on the entire frame tree. This is a little
> > kludgey,
> > > > but
> > > > > > we
> > > > > > > > can assert that the node passed in is the main frame node and
call
> > it
> > > > > > > something
> > > > > > > > like "should_recur_on_main_frame."
> > > > > > > > 
> > > > > > > > 
> > > > > > > > This gives us the following new API
> > > > > > > > 
> > > > > > > > content/public
> > > > > > > > BlockRequestsForFrame(framehost, recur)
> > > > > > > > ResumeRequestsForFrame(framehost, recur)
> > > > > > > > 
> > > > > > > > content/
> > > > > > > > CancelBlockedRequestsForFrame(framehost, recur)
> > > > > > > > BlockRequestsForRoute(route)
> > > > > > > > ResumeBlockedRequestsForRoute(route)
> > > > > > > > 
> > > > > > > > The ForRoute methods will be used by the WebContentsImpl
NewWindow
> > > code
> > > > > path
> > > > > > > > only (no framehost available on UI thread, other call is on IO
> > > thread).
> > > > > > > > 
> > > > > > > > WDYT? I'll start implementing when you give signoff. The other
> > problem
> > > > > with
> > > > > > > this
> > > > > > > > API is that it gives content/public access to the FrameTree API,
> so
> > we
> > > > > could
> > > > > > > > also do the following:
> > > > > > > 
> > > > > > > So I like the above API quite a bit; thank you.  Having said that,
> > > unless
> > > > > the
> > > > > > > API's going to be much worse without, I'd lean against adding
stuff
> to
> > > the
> > > > > > > content/ interface (== content/public), and you should certainly
run
> > the
> > > > > > change
> > > > > > > by a content/ OWNER before implementing, as that's exactly the
area
> > that
> > > > > they
> > > > > > > tend to be most conservative around.  Having said *that*, I don't
> see
> > > why
> > > > > you
> > > > > > > need to add the FrameTree concept to the interface?  It's just
> adding
> > a
> > > > > > boolean
> > > > > > > to the interface you sketch out below.  (If what you mean is
giving
> > the
> > > > > > > *implementation* of methods in content/public access to the
> FrameTree
> > > API,
> > > > I
> > > > > > > think that's ok, and also avoidable if it's not (just indirect
> > directly
> > > > into
> > > > > > the
> > > > > > > same static on the RDHI) but you should probably check with a
> content/
> > > > OWNER
> > > > > > > just in case).
> > > > > > > 
> > > > > > > > content/public
> > > > > > > > BlockRequestsForFrame(framehost)
> > > > > > > > ResumeRequestsForFrame(framehost)
> > > > > > > > 
> > > > > > > > content/
> > > > > > > > BlockRequestsForFrame(framehost, recur)
> > > > > > > > ResumeRequestsForFrame(framehost, recur)
> > > > > > > 
> > > > > > > I think having the same name but different signatures on static
> > methods
> > > of
> > > > > two
> > > > > > > classes that have an inheritance relationship is an awesome recipe
> for
> > > > > > confusion
> > > > > > > and mistakes; I'd recommend against :-}.
> > > > > > > 
> > > > > > > > CancelBlockedRequestsForFrame(framehost, recur)
> > > > > > > > BlockRequestsForRoute(route)
> > > > > > > > ResumeBlockedRequestsForRoute(route)
> > > > > > > > 
> > > > > > > > But that's worse than two APIs that take FrameHosts and
> FrameTrees,
> > > > > > > > respectively.
> > > > > > 
> > > > > > Yeah it would be the implementation (i.e. they just take a bool).
> > > Consumers
> > > > > > won't need access to FrameTrees. If that's too much I guess the next
> > best
> > > > > thing
> > > > > > is
> > > > > 
> > > > > I think an implementation dependency is fine, actually.  So +1 for
your
> > > first
> > > > > API plan.
> > > > 
> > > > Just to be explicit, can you list which is the last standing proposal?
> > > 
> > > I am in favor of the first API described in c#21.
> > > 
> > > > In general, we don't want to expose FrameTree outside of content/.
> > > 
> > > This exposes the tuple (RenderFrameHost, bool) outside of content/, which
I
> > > presume is ok.  It's conceptually exposing the frame tree (the bool
> indicates
> > > whether you'll recurse down it or not) but not exposing that type.  I
> presume
> > > that's ok, since I presume the concept of frames having frames in them is
> > sorta
> > > obvious, but I wanted a content/ OWNER's opinion (i.e. your opinion) on it
> as
> > > well.
> > 
> > My only question is why do we need the recurse parameter? I have to look at
> all
> > the call sites for the new API, but in general I think we use it when doing
> > navigations, right? If that is the case, navigating any frame ends up
> destroying
> > all of its subframes, so no need to recurse into them. Also, we use it when
> > creating new WebContents, which by definition will only have one frame and
> will
> > not have subframes until we have unblocked the loads so a document can be
> > loaded.
> > 
> > > -- Randy
> > > 
> > > >  
> > > > > > content/public
> > > > > > BlockRequestsForFrame(framehost)
> > > > > > ResumeRequestsForFrame(framehost)
> > > > > > 
> > > > > > content/
> > > > > > BlockRequestsForFrameTree(tree)
> > > > > > ResumeRequestsForFrameTree(tree)
> > > > > > CancelBlockedRequestsForFrameTree(tree)
> > > > > > BlockRequestsForRoute(route)
> > > > > > ResumeBlockedRequestsForRoute(route)
> 
> The only time we need it is for interstitials blocking their underlying page.
> I'm fine with having a special (non content/public) interface for them if you
> think that's a cleaner solution.

Now that Nasko's weighed in I find myself interested in this issue as well. 
When would we ever want to block requests on the top level frame and *not* on
all its subframes?  I.e. can't we just always take recurse as true?

Charlie Harrison

On 2016/01/27 20:17:02, Randy Smith - Not in Fridays wrote: > On 2016/01/27 19:31:18, csharrison ...

4 years, 11 months ago (2016-01-27 20:28:28 UTC) #30

On 2016/01/27 20:17:02, Randy Smith - Not in Fridays wrote:
> On 2016/01/27 19:31:18, csharrison wrote:
> > On 2016/01/27 19:28:37, nasko wrote:
> > > On 2016/01/27 19:13:57, Randy Smith - Not in Fridays wrote:
> > > > On 2016/01/27 18:31:39, nasko wrote:
> > > > > On 2016/01/26 20:49:51, Randy Smith - Not in Fridays wrote:
> > > > > > On 2016/01/26 20:48:10, csharrison wrote:
> > > > > > > On 2016/01/26 20:32:12, Randy Smith - Not in Fridays wrote:
> > > > > > > > On 2016/01/25 16:13:47, csharrison wrote:
> > > > > > > > > On 2016/01/22 17:38:59, Randy Smith - Not in Fridays wrote:
> > > > > > > > > > On 2016/01/21 18:52:56, csharrison wrote:
> > > > > > > > > > > Responded to comments without making architectural
changes.
> > > > > Hopefully
> > > > > > > will
> > > > > > > > > > > decide w/ Randy tomorrow how to structure this and shoot
up
> > > > another
> > > > > CL
> > > > > > > by
> > > > > > > > > the
> > > > > > > > > > > weekend. We'll see.
> > > > > > > > > > 
> > > > > > > > > > Just to make sure we're on the same page based on our
> > conversation
> > > > > > > offline;
> > > > > > > > > I'm
> > > > > > > > > > withdrawing my comments about the RDH static methods, but
the
> > rest
> > > > of
> > > > > my
> > > > > > > > > > architectural comments/suggestions stand, and you don't need
> > > another
> > > > > > > review
> > > > > > > > > from
> > > > > > > > > > me until you've responded to those.  Let me know if any of
> that
> > > > > doesn't
> > > > > > > > match
> > > > > > > > > > your perspective.
> > > > > > > > > 
> > > > > > > > > Yep, on the same page. This is the best revamped API I can
come
> up
> > > > with:
> > > > > > > > > 
> > > > > > > > > 1. Remove NotifyFor/NotifyForEach. This entails creating
> > > > > > Cancel/Block/Resume
> > > > > > > > for
> > > > > > > > > Frame / Route methods.
> > > > > > > > > 
> > > > > > > > > 2. Add a boolean to Cancel/Block/ResumeForFrame method
> indicating
> > > that
> > > > > the
> > > > > > > > > method should work on the entire frame tree. This is a little
> > > kludgey,
> > > > > but
> > > > > > > we
> > > > > > > > > can assert that the node passed in is the main frame node and
> call
> > > it
> > > > > > > > something
> > > > > > > > > like "should_recur_on_main_frame."
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > This gives us the following new API
> > > > > > > > > 
> > > > > > > > > content/public
> > > > > > > > > BlockRequestsForFrame(framehost, recur)
> > > > > > > > > ResumeRequestsForFrame(framehost, recur)
> > > > > > > > > 
> > > > > > > > > content/
> > > > > > > > > CancelBlockedRequestsForFrame(framehost, recur)
> > > > > > > > > BlockRequestsForRoute(route)
> > > > > > > > > ResumeBlockedRequestsForRoute(route)
> > > > > > > > > 
> > > > > > > > > The ForRoute methods will be used by the WebContentsImpl
> NewWindow
> > > > code
> > > > > > path
> > > > > > > > > only (no framehost available on UI thread, other call is on IO
> > > > thread).
> > > > > > > > > 
> > > > > > > > > WDYT? I'll start implementing when you give signoff. The other
> > > problem
> > > > > > with
> > > > > > > > this
> > > > > > > > > API is that it gives content/public access to the FrameTree
API,
> > so
> > > we
> > > > > > could
> > > > > > > > > also do the following:
> > > > > > > > 
> > > > > > > > So I like the above API quite a bit; thank you.  Having said
that,
> > > > unless
> > > > > > the
> > > > > > > > API's going to be much worse without, I'd lean against adding
> stuff
> > to
> > > > the
> > > > > > > > content/ interface (== content/public), and you should certainly
> run
> > > the
> > > > > > > change
> > > > > > > > by a content/ OWNER before implementing, as that's exactly the
> area
> > > that
> > > > > > they
> > > > > > > > tend to be most conservative around.  Having said *that*, I
don't
> > see
> > > > why
> > > > > > you
> > > > > > > > need to add the FrameTree concept to the interface?  It's just
> > adding
> > > a
> > > > > > > boolean
> > > > > > > > to the interface you sketch out below.  (If what you mean is
> giving
> > > the
> > > > > > > > *implementation* of methods in content/public access to the
> > FrameTree
> > > > API,
> > > > > I
> > > > > > > > think that's ok, and also avoidable if it's not (just indirect
> > > directly
> > > > > into
> > > > > > > the
> > > > > > > > same static on the RDHI) but you should probably check with a
> > content/
> > > > > OWNER
> > > > > > > > just in case).
> > > > > > > > 
> > > > > > > > > content/public
> > > > > > > > > BlockRequestsForFrame(framehost)
> > > > > > > > > ResumeRequestsForFrame(framehost)
> > > > > > > > > 
> > > > > > > > > content/
> > > > > > > > > BlockRequestsForFrame(framehost, recur)
> > > > > > > > > ResumeRequestsForFrame(framehost, recur)
> > > > > > > > 
> > > > > > > > I think having the same name but different signatures on static
> > > methods
> > > > of
> > > > > > two
> > > > > > > > classes that have an inheritance relationship is an awesome
recipe
> > for
> > > > > > > confusion
> > > > > > > > and mistakes; I'd recommend against :-}.
> > > > > > > > 
> > > > > > > > > CancelBlockedRequestsForFrame(framehost, recur)
> > > > > > > > > BlockRequestsForRoute(route)
> > > > > > > > > ResumeBlockedRequestsForRoute(route)
> > > > > > > > > 
> > > > > > > > > But that's worse than two APIs that take FrameHosts and
> > FrameTrees,
> > > > > > > > > respectively.
> > > > > > > 
> > > > > > > Yeah it would be the implementation (i.e. they just take a bool).
> > > > Consumers
> > > > > > > won't need access to FrameTrees. If that's too much I guess the
next
> > > best
> > > > > > thing
> > > > > > > is
> > > > > > 
> > > > > > I think an implementation dependency is fine, actually.  So +1 for
> your
> > > > first
> > > > > > API plan.
> > > > > 
> > > > > Just to be explicit, can you list which is the last standing proposal?
> > > > 
> > > > I am in favor of the first API described in c#21.
> > > > 
> > > > > In general, we don't want to expose FrameTree outside of content/.
> > > > 
> > > > This exposes the tuple (RenderFrameHost, bool) outside of content/,
which
> I
> > > > presume is ok.  It's conceptually exposing the frame tree (the bool
> > indicates
> > > > whether you'll recurse down it or not) but not exposing that type.  I
> > presume
> > > > that's ok, since I presume the concept of frames having frames in them
is
> > > sorta
> > > > obvious, but I wanted a content/ OWNER's opinion (i.e. your opinion) on
it
> > as
> > > > well.
> > > 
> > > My only question is why do we need the recurse parameter? I have to look
at
> > all
> > > the call sites for the new API, but in general I think we use it when
doing
> > > navigations, right? If that is the case, navigating any frame ends up
> > destroying
> > > all of its subframes, so no need to recurse into them. Also, we use it
when
> > > creating new WebContents, which by definition will only have one frame and
> > will
> > > not have subframes until we have unblocked the loads so a document can be
> > > loaded.
> > > 
> > > > -- Randy
> > > > 
> > > > >  
> > > > > > > content/public
> > > > > > > BlockRequestsForFrame(framehost)
> > > > > > > ResumeRequestsForFrame(framehost)
> > > > > > > 
> > > > > > > content/
> > > > > > > BlockRequestsForFrameTree(tree)
> > > > > > > ResumeRequestsForFrameTree(tree)
> > > > > > > CancelBlockedRequestsForFrameTree(tree)
> > > > > > > BlockRequestsForRoute(route)
> > > > > > > ResumeBlockedRequestsForRoute(route)
> > 
> > The only time we need it is for interstitials blocking their underlying
page.
> > I'm fine with having a special (non content/public) interface for them if
you
> > think that's a cleaner solution.
> 
> Now that Nasko's weighed in I find myself interested in this issue as well. 
> When would we ever want to block requests on the top level frame and *not* on
> all its subframes?  I.e. can't we just always take recurse as true?

Good point. I think we can. I was opting not to for a kind of "principle of
least power." I imagine that should work, and it ties cleaner with the old
implementation keying off rvid.

Randy Smith (Not in Mondays)

On 2016/01/27 20:28:28, csharrison wrote: > On 2016/01/27 20:17:02, Randy Smith - Not in Fridays ...

4 years, 11 months ago (2016-01-27 20:31:59 UTC) #31

On 2016/01/27 20:28:28, csharrison wrote:
> On 2016/01/27 20:17:02, Randy Smith - Not in Fridays wrote:
> > On 2016/01/27 19:31:18, csharrison wrote:
> > > On 2016/01/27 19:28:37, nasko wrote:
> > > > On 2016/01/27 19:13:57, Randy Smith - Not in Fridays wrote:
> > > > > On 2016/01/27 18:31:39, nasko wrote:
> > > > > > On 2016/01/26 20:49:51, Randy Smith - Not in Fridays wrote:
> > > > > > > On 2016/01/26 20:48:10, csharrison wrote:
> > > > > > > > On 2016/01/26 20:32:12, Randy Smith - Not in Fridays wrote:
> > > > > > > > > On 2016/01/25 16:13:47, csharrison wrote:
> > > > > > > > > > On 2016/01/22 17:38:59, Randy Smith - Not in Fridays wrote:
> > > > > > > > > > > On 2016/01/21 18:52:56, csharrison wrote:
> > > > > > > > > > > > Responded to comments without making architectural
> changes.
> > > > > > Hopefully
> > > > > > > > will
> > > > > > > > > > > > decide w/ Randy tomorrow how to structure this and shoot
> up
> > > > > another
> > > > > > CL
> > > > > > > > by
> > > > > > > > > > the
> > > > > > > > > > > > weekend. We'll see.
> > > > > > > > > > > 
> > > > > > > > > > > Just to make sure we're on the same page based on our
> > > conversation
> > > > > > > > offline;
> > > > > > > > > > I'm
> > > > > > > > > > > withdrawing my comments about the RDH static methods, but
> the
> > > rest
> > > > > of
> > > > > > my
> > > > > > > > > > > architectural comments/suggestions stand, and you don't
need
> > > > another
> > > > > > > > review
> > > > > > > > > > from
> > > > > > > > > > > me until you've responded to those.  Let me know if any of
> > that
> > > > > > doesn't
> > > > > > > > > match
> > > > > > > > > > > your perspective.
> > > > > > > > > > 
> > > > > > > > > > Yep, on the same page. This is the best revamped API I can
> come
> > up
> > > > > with:
> > > > > > > > > > 
> > > > > > > > > > 1. Remove NotifyFor/NotifyForEach. This entails creating
> > > > > > > Cancel/Block/Resume
> > > > > > > > > for
> > > > > > > > > > Frame / Route methods.
> > > > > > > > > > 
> > > > > > > > > > 2. Add a boolean to Cancel/Block/ResumeForFrame method
> > indicating
> > > > that
> > > > > > the
> > > > > > > > > > method should work on the entire frame tree. This is a
little
> > > > kludgey,
> > > > > > but
> > > > > > > > we
> > > > > > > > > > can assert that the node passed in is the main frame node
and
> > call
> > > > it
> > > > > > > > > something
> > > > > > > > > > like "should_recur_on_main_frame."
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > This gives us the following new API
> > > > > > > > > > 
> > > > > > > > > > content/public
> > > > > > > > > > BlockRequestsForFrame(framehost, recur)
> > > > > > > > > > ResumeRequestsForFrame(framehost, recur)
> > > > > > > > > > 
> > > > > > > > > > content/
> > > > > > > > > > CancelBlockedRequestsForFrame(framehost, recur)
> > > > > > > > > > BlockRequestsForRoute(route)
> > > > > > > > > > ResumeBlockedRequestsForRoute(route)
> > > > > > > > > > 
> > > > > > > > > > The ForRoute methods will be used by the WebContentsImpl
> > NewWindow
> > > > > code
> > > > > > > path
> > > > > > > > > > only (no framehost available on UI thread, other call is on
IO
> > > > > thread).
> > > > > > > > > > 
> > > > > > > > > > WDYT? I'll start implementing when you give signoff. The
other
> > > > problem
> > > > > > > with
> > > > > > > > > this
> > > > > > > > > > API is that it gives content/public access to the FrameTree
> API,
> > > so
> > > > we
> > > > > > > could
> > > > > > > > > > also do the following:
> > > > > > > > > 
> > > > > > > > > So I like the above API quite a bit; thank you.  Having said
> that,
> > > > > unless
> > > > > > > the
> > > > > > > > > API's going to be much worse without, I'd lean against adding
> > stuff
> > > to
> > > > > the
> > > > > > > > > content/ interface (== content/public), and you should
certainly
> > run
> > > > the
> > > > > > > > change
> > > > > > > > > by a content/ OWNER before implementing, as that's exactly the
> > area
> > > > that
> > > > > > > they
> > > > > > > > > tend to be most conservative around.  Having said *that*, I
> don't
> > > see
> > > > > why
> > > > > > > you
> > > > > > > > > need to add the FrameTree concept to the interface?  It's just
> > > adding
> > > > a
> > > > > > > > boolean
> > > > > > > > > to the interface you sketch out below.  (If what you mean is
> > giving
> > > > the
> > > > > > > > > *implementation* of methods in content/public access to the
> > > FrameTree
> > > > > API,
> > > > > > I
> > > > > > > > > think that's ok, and also avoidable if it's not (just indirect
> > > > directly
> > > > > > into
> > > > > > > > the
> > > > > > > > > same static on the RDHI) but you should probably check with a
> > > content/
> > > > > > OWNER
> > > > > > > > > just in case).
> > > > > > > > > 
> > > > > > > > > > content/public
> > > > > > > > > > BlockRequestsForFrame(framehost)
> > > > > > > > > > ResumeRequestsForFrame(framehost)
> > > > > > > > > > 
> > > > > > > > > > content/
> > > > > > > > > > BlockRequestsForFrame(framehost, recur)
> > > > > > > > > > ResumeRequestsForFrame(framehost, recur)
> > > > > > > > > 
> > > > > > > > > I think having the same name but different signatures on
static
> > > > methods
> > > > > of
> > > > > > > two
> > > > > > > > > classes that have an inheritance relationship is an awesome
> recipe
> > > for
> > > > > > > > confusion
> > > > > > > > > and mistakes; I'd recommend against :-}.
> > > > > > > > > 
> > > > > > > > > > CancelBlockedRequestsForFrame(framehost, recur)
> > > > > > > > > > BlockRequestsForRoute(route)
> > > > > > > > > > ResumeBlockedRequestsForRoute(route)
> > > > > > > > > > 
> > > > > > > > > > But that's worse than two APIs that take FrameHosts and
> > > FrameTrees,
> > > > > > > > > > respectively.
> > > > > > > > 
> > > > > > > > Yeah it would be the implementation (i.e. they just take a
bool).
> > > > > Consumers
> > > > > > > > won't need access to FrameTrees. If that's too much I guess the
> next
> > > > best
> > > > > > > thing
> > > > > > > > is
> > > > > > > 
> > > > > > > I think an implementation dependency is fine, actually.  So +1 for
> > your
> > > > > first
> > > > > > > API plan.
> > > > > > 
> > > > > > Just to be explicit, can you list which is the last standing
proposal?
> > > > > 
> > > > > I am in favor of the first API described in c#21.
> > > > > 
> > > > > > In general, we don't want to expose FrameTree outside of content/.
> > > > > 
> > > > > This exposes the tuple (RenderFrameHost, bool) outside of content/,
> which
> > I
> > > > > presume is ok.  It's conceptually exposing the frame tree (the bool
> > > indicates
> > > > > whether you'll recurse down it or not) but not exposing that type.  I
> > > presume
> > > > > that's ok, since I presume the concept of frames having frames in them
> is
> > > > sorta
> > > > > obvious, but I wanted a content/ OWNER's opinion (i.e. your opinion)
on
> it
> > > as
> > > > > well.
> > > > 
> > > > My only question is why do we need the recurse parameter? I have to look
> at
> > > all
> > > > the call sites for the new API, but in general I think we use it when
> doing
> > > > navigations, right? If that is the case, navigating any frame ends up
> > > destroying
> > > > all of its subframes, so no need to recurse into them. Also, we use it
> when
> > > > creating new WebContents, which by definition will only have one frame
and
> > > will
> > > > not have subframes until we have unblocked the loads so a document can
be
> > > > loaded.
> > > > 
> > > > > -- Randy
> > > > > 
> > > > > >  
> > > > > > > > content/public
> > > > > > > > BlockRequestsForFrame(framehost)
> > > > > > > > ResumeRequestsForFrame(framehost)
> > > > > > > > 
> > > > > > > > content/
> > > > > > > > BlockRequestsForFrameTree(tree)
> > > > > > > > ResumeRequestsForFrameTree(tree)
> > > > > > > > CancelBlockedRequestsForFrameTree(tree)
> > > > > > > > BlockRequestsForRoute(route)
> > > > > > > > ResumeBlockedRequestsForRoute(route)
> > > 
> > > The only time we need it is for interstitials blocking their underlying
> page.
> > > I'm fine with having a special (non content/public) interface for them if
> you
> > > think that's a cleaner solution.
> > 
> > Now that Nasko's weighed in I find myself interested in this issue as well. 
> > When would we ever want to block requests on the top level frame and *not*
on
> > all its subframes?  I.e. can't we just always take recurse as true?
> 
> Good point. I think we can. I was opting not to for a kind of "principle of
> least power." I imagine that should work, and it ties cleaner with the old
> implementation keying off rvid.

I think the heuristic I'd advocate for in this space is "don't add complexity
until it's needed".  But which heuristic should trump is always a tricky
judgement.

Charlie Harrison

Description was changed from ========== Refactored blocked_loaders_map_ to key by render frame route id This ...

4 years, 10 months ago (2016-01-29 16:11:30 UTC) #32

Charlie Harrison

On 2016/01/27 at 20:31:59, rdsmith wrote: > On 2016/01/27 20:28:28, csharrison wrote: > > On ...

4 years, 10 months ago (2016-01-29 19:36:27 UTC) #33

On 2016/01/27 at 20:31:59, rdsmith wrote:
> On 2016/01/27 20:28:28, csharrison wrote:
> > On 2016/01/27 20:17:02, Randy Smith - Not in Fridays wrote:
> > > On 2016/01/27 19:31:18, csharrison wrote:
> > > > On 2016/01/27 19:28:37, nasko wrote:
> > > > > On 2016/01/27 19:13:57, Randy Smith - Not in Fridays wrote:
> > > > > > On 2016/01/27 18:31:39, nasko wrote:
> > > > > > > On 2016/01/26 20:49:51, Randy Smith - Not in Fridays wrote:
> > > > > > > > On 2016/01/26 20:48:10, csharrison wrote:
> > > > > > > > > On 2016/01/26 20:32:12, Randy Smith - Not in Fridays wrote:
> > > > > > > > > > On 2016/01/25 16:13:47, csharrison wrote:
> > > > > > > > > > > On 2016/01/22 17:38:59, Randy Smith - Not in Fridays
wrote:
> > > > > > > > > > > > On 2016/01/21 18:52:56, csharrison wrote:
> > > > > > > > > > > > > Responded to comments without making architectural
> > changes.
> > > > > > > Hopefully
> > > > > > > > > will
> > > > > > > > > > > > > decide w/ Randy tomorrow how to structure this and
shoot
> > up
> > > > > > another
> > > > > > > CL
> > > > > > > > > by
> > > > > > > > > > > the
> > > > > > > > > > > > > weekend. We'll see.
> > > > > > > > > > > > 
> > > > > > > > > > > > Just to make sure we're on the same page based on our
> > > > conversation
> > > > > > > > > offline;
> > > > > > > > > > > I'm
> > > > > > > > > > > > withdrawing my comments about the RDH static methods,
but
> > the
> > > > rest
> > > > > > of
> > > > > > > my
> > > > > > > > > > > > architectural comments/suggestions stand, and you don't
need
> > > > > another
> > > > > > > > > review
> > > > > > > > > > > from
> > > > > > > > > > > > me until you've responded to those.  Let me know if any
of
> > > that
> > > > > > > doesn't
> > > > > > > > > > match
> > > > > > > > > > > > your perspective.
> > > > > > > > > > > 
> > > > > > > > > > > Yep, on the same page. This is the best revamped API I can
> > come
> > > up
> > > > > > with:
> > > > > > > > > > > 
> > > > > > > > > > > 1. Remove NotifyFor/NotifyForEach. This entails creating
> > > > > > > > Cancel/Block/Resume
> > > > > > > > > > for
> > > > > > > > > > > Frame / Route methods.
> > > > > > > > > > > 
> > > > > > > > > > > 2. Add a boolean to Cancel/Block/ResumeForFrame method
> > > indicating
> > > > > that
> > > > > > > the
> > > > > > > > > > > method should work on the entire frame tree. This is a
little
> > > > > kludgey,
> > > > > > > but
> > > > > > > > > we
> > > > > > > > > > > can assert that the node passed in is the main frame node
and
> > > call
> > > > > it
> > > > > > > > > > something
> > > > > > > > > > > like "should_recur_on_main_frame."
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > This gives us the following new API
> > > > > > > > > > > 
> > > > > > > > > > > content/public
> > > > > > > > > > > BlockRequestsForFrame(framehost, recur)
> > > > > > > > > > > ResumeRequestsForFrame(framehost, recur)
> > > > > > > > > > > 
> > > > > > > > > > > content/
> > > > > > > > > > > CancelBlockedRequestsForFrame(framehost, recur)
> > > > > > > > > > > BlockRequestsForRoute(route)
> > > > > > > > > > > ResumeBlockedRequestsForRoute(route)
> > > > > > > > > > > 
> > > > > > > > > > > The ForRoute methods will be used by the WebContentsImpl
> > > NewWindow
> > > > > > code
> > > > > > > > path
> > > > > > > > > > > only (no framehost available on UI thread, other call is
on IO
> > > > > > thread).
> > > > > > > > > > > 
> > > > > > > > > > > WDYT? I'll start implementing when you give signoff. The
other
> > > > > problem
> > > > > > > > with
> > > > > > > > > > this
> > > > > > > > > > > API is that it gives content/public access to the
FrameTree
> > API,
> > > > so
> > > > > we
> > > > > > > > could
> > > > > > > > > > > also do the following:
> > > > > > > > > > 
> > > > > > > > > > So I like the above API quite a bit; thank you.  Having said
> > that,
> > > > > > unless
> > > > > > > > the
> > > > > > > > > > API's going to be much worse without, I'd lean against
adding
> > > stuff
> > > > to
> > > > > > the
> > > > > > > > > > content/ interface (== content/public), and you should
certainly
> > > run
> > > > > the
> > > > > > > > > change
> > > > > > > > > > by a content/ OWNER before implementing, as that's exactly
the
> > > area
> > > > > that
> > > > > > > > they
> > > > > > > > > > tend to be most conservative around.  Having said *that*, I
> > don't
> > > > see
> > > > > > why
> > > > > > > > you
> > > > > > > > > > need to add the FrameTree concept to the interface?  It's
just
> > > > adding
> > > > > a
> > > > > > > > > boolean
> > > > > > > > > > to the interface you sketch out below.  (If what you mean is
> > > giving
> > > > > the
> > > > > > > > > > *implementation* of methods in content/public access to the
> > > > FrameTree
> > > > > > API,
> > > > > > > I
> > > > > > > > > > think that's ok, and also avoidable if it's not (just
indirect
> > > > > directly
> > > > > > > into
> > > > > > > > > the
> > > > > > > > > > same static on the RDHI) but you should probably check with
a
> > > > content/
> > > > > > > OWNER
> > > > > > > > > > just in case).
> > > > > > > > > > 
> > > > > > > > > > > content/public
> > > > > > > > > > > BlockRequestsForFrame(framehost)
> > > > > > > > > > > ResumeRequestsForFrame(framehost)
> > > > > > > > > > > 
> > > > > > > > > > > content/
> > > > > > > > > > > BlockRequestsForFrame(framehost, recur)
> > > > > > > > > > > ResumeRequestsForFrame(framehost, recur)
> > > > > > > > > > 
> > > > > > > > > > I think having the same name but different signatures on
static
> > > > > methods
> > > > > > of
> > > > > > > > two
> > > > > > > > > > classes that have an inheritance relationship is an awesome
> > recipe
> > > > for
> > > > > > > > > confusion
> > > > > > > > > > and mistakes; I'd recommend against :-}.
> > > > > > > > > > 
> > > > > > > > > > > CancelBlockedRequestsForFrame(framehost, recur)
> > > > > > > > > > > BlockRequestsForRoute(route)
> > > > > > > > > > > ResumeBlockedRequestsForRoute(route)
> > > > > > > > > > > 
> > > > > > > > > > > But that's worse than two APIs that take FrameHosts and
> > > > FrameTrees,
> > > > > > > > > > > respectively.
> > > > > > > > > 
> > > > > > > > > Yeah it would be the implementation (i.e. they just take a
bool).
> > > > > > Consumers
> > > > > > > > > won't need access to FrameTrees. If that's too much I guess
the
> > next
> > > > > best
> > > > > > > > thing
> > > > > > > > > is
> > > > > > > > 
> > > > > > > > I think an implementation dependency is fine, actually.  So +1
for
> > > your
> > > > > > first
> > > > > > > > API plan.
> > > > > > > 
> > > > > > > Just to be explicit, can you list which is the last standing
proposal?
> > > > > > 
> > > > > > I am in favor of the first API described in c#21.
> > > > > > 
> > > > > > > In general, we don't want to expose FrameTree outside of content/.
> > > > > > 
> > > > > > This exposes the tuple (RenderFrameHost, bool) outside of content/,
> > which
> > > I
> > > > > > presume is ok.  It's conceptually exposing the frame tree (the bool
> > > > indicates
> > > > > > whether you'll recurse down it or not) but not exposing that type. 
I
> > > > presume
> > > > > > that's ok, since I presume the concept of frames having frames in
them
> > is
> > > > > sorta
> > > > > > obvious, but I wanted a content/ OWNER's opinion (i.e. your opinion)
on
> > it
> > > > as
> > > > > > well.
> > > > > 
> > > > > My only question is why do we need the recurse parameter? I have to
look
> > at
> > > > all
> > > > > the call sites for the new API, but in general I think we use it when
> > doing
> > > > > navigations, right? If that is the case, navigating any frame ends up
> > > > destroying
> > > > > all of its subframes, so no need to recurse into them. Also, we use it
> > when
> > > > > creating new WebContents, which by definition will only have one frame
and
> > > > will
> > > > > not have subframes until we have unblocked the loads so a document can
be
> > > > > loaded.
> > > > > 
> > > > > > -- Randy
> > > > > > 
> > > > > > >  
> > > > > > > > > content/public
> > > > > > > > > BlockRequestsForFrame(framehost)
> > > > > > > > > ResumeRequestsForFrame(framehost)
> > > > > > > > > 
> > > > > > > > > content/
> > > > > > > > > BlockRequestsForFrameTree(tree)
> > > > > > > > > ResumeRequestsForFrameTree(tree)
> > > > > > > > > CancelBlockedRequestsForFrameTree(tree)
> > > > > > > > > BlockRequestsForRoute(route)
> > > > > > > > > ResumeBlockedRequestsForRoute(route)
> > > > 
> > > > The only time we need it is for interstitials blocking their underlying
> > page.
> > > > I'm fine with having a special (non content/public) interface for them
if
> > you
> > > > think that's a cleaner solution.
> > > 
> > > Now that Nasko's weighed in I find myself interested in this issue as
well. 
> > > When would we ever want to block requests on the top level frame and *not*
on
> > > all its subframes?  I.e. can't we just always take recurse as true?
> > 
> > Good point. I think we can. I was opting not to for a kind of "principle of
> > least power." I imagine that should work, and it ties cleaner with the old
> > implementation keying off rvid.
> 
> I think the heuristic I'd advocate for in this space is "don't add complexity
until it's needed".  But which heuristic should trump is always a tricky
judgement.

PTAL. Looks like initial trybots are okay with the recursive-by-default API for
UI thread blocking/resuming.

Randy Smith (Not in Mondays)

This looks good; thank you. Mostly nits below. https://codereview.chromium.org/1542743002/diff/380001/content/browser/loader/resource_dispatcher_host_impl.cc File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1542743002/diff/380001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode493 content/browser/loader/resource_dispatcher_host_impl.cc:493: void ...

4 years, 10 months ago (2016-02-01 21:19:20 UTC) #34

This looks good; thank you.  Mostly nits below.

https://codereview.chromium.org/1542743002/diff/380001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/1542743002/diff/380001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:493: void
NotifyForEachFrameOnIO(
nitty nit, suggestion: The name of this function implies it's more different
from NotifyForRouteOnIO than it is; basically it's the plural of
NotifyForRouteOnIO and nothing more (they both take frame routing ids).  So
maybe NotifyForFrameListOnIO?  But up to you; it's a really small point (and I'm
a lot less concerned with the naming of routines that aren't part of the class
interface).

https://codereview.chromium.org/1542743002/diff/380001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:502: void
NotifyForEachFrame(
Same as above.

https://codereview.chromium.org/1542743002/diff/380001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:540:
base::Bind(&ResourceDispatcherHostImpl::OnRenderFrameDeleted));
nit, suggestion: I'm inclined to think it's a cleaner, more minimalist interface
to mark LoaderIOThreadNotifier a friend of RDHI and make
RDHI::OnRenderFrameDeleted private.  Why invite other classes to call it?

https://codereview.chromium.org/1542743002/diff/380001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1874:
CancelRequestsForRoute(global_routing_id);
Alternatively to my other suggestion, just make LoaderIOThreadNotifier a friend
and have it post to CancelRequestsForRoute directly.

https://codereview.chromium.org/1542743002/diff/380001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.h (right):

https://codereview.chromium.org/1542743002/diff/380001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.h:114: static void
ResumeBlockedRequestsForRouteOnUI(
Nit: For consistency, I'd either name them all with the thread they need to be
called on, or name none of them with that thread.  

ETA: Thinking about it after reading through the implementation, I'd name them
all with the thread.  I think I'd suggest (up to you) that it be *FromUI rather
than *OnUI, just because what *OnUI means to me is that the routine is a
delegate from some other thread running the operation named On the given thread,
but it's secondary/implementation.  *FromUI suggests that it's unusual, but
(again to me) not done on behalf of some other entity.

A lot of what's going on for me here is that there are a lot of interfaces on
RDHI already which have similar names to many of these, and I want to minimize
confusion going forward from callers as to what's ok to call from what thread.

Charlie Harrison

Thanks for the review! https://codereview.chromium.org/1542743002/diff/380001/content/browser/loader/resource_dispatcher_host_impl.cc File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1542743002/diff/380001/content/browser/loader/resource_dispatcher_host_impl.cc#newcode493 content/browser/loader/resource_dispatcher_host_impl.cc:493: void NotifyForEachFrameOnIO( On 2016/02/01 21:19:20, ...

4 years, 10 months ago (2016-02-01 23:05:39 UTC) #35

Thanks for the review!

https://codereview.chromium.org/1542743002/diff/380001/content/browser/loader...
File content/browser/loader/resource_dispatcher_host_impl.cc (right):

https://codereview.chromium.org/1542743002/diff/380001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:493: void
NotifyForEachFrameOnIO(
On 2016/02/01 21:19:20, Randy Smith - Not in Fridays wrote:
> nitty nit, suggestion: The name of this function implies it's more different
> from NotifyForRouteOnIO than it is; basically it's the plural of
> NotifyForRouteOnIO and nothing more (they both take frame routing ids).  So
> maybe NotifyForFrameListOnIO?  But up to you; it's a really small point (and
I'm
> a lot less concerned with the naming of routines that aren't part of the class
> interface).

Changed it to "NotifyForRouteSetOnIO", to better parallel NotifyForRoute.

https://codereview.chromium.org/1542743002/diff/380001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:502: void
NotifyForEachFrame(
On 2016/02/01 21:19:20, Randy Smith - Not in Fridays wrote:
> Same as above.  

I prefer to leave this one as is. I can't really think of anything better at the
moment.

https://codereview.chromium.org/1542743002/diff/380001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:540:
base::Bind(&ResourceDispatcherHostImpl::OnRenderFrameDeleted));
On 2016/02/01 21:19:20, Randy Smith - Not in Fridays wrote:
> nit, suggestion: I'm inclined to think it's a cleaner, more minimalist
interface
> to mark LoaderIOThreadNotifier a friend of RDHI and make
> RDHI::OnRenderFrameDeleted private.  Why invite other classes to call it?

I implemented this suggestion.

https://codereview.chromium.org/1542743002/diff/380001/content/browser/loader...
content/browser/loader/resource_dispatcher_host_impl.cc:1874:
CancelRequestsForRoute(global_routing_id);
On 2016/02/01 21:19:20, Randy Smith - Not in Fridays wrote:
> Alternatively to my other suggestion, just make LoaderIOThreadNotifier a
friend
> and have it post to CancelRequestsForRoute directly.

I implemented your previous suggestion.

Randy Smith (Not in Mondays)

LGTM (you can do what you like with the comment below). https://codereview.chromium.org/1542743002/diff/380001/content/browser/loader/resource_dispatcher_host_impl.cc File content/browser/loader/resource_dispatcher_host_impl.cc (right): ...

4 years, 10 months ago (2016-02-02 20:47:06 UTC) #36

Charlie Harrison

Thanks :) https://codereview.chromium.org/1542743002/diff/420001/content/browser/web_contents/web_contents_impl.cc File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/1542743002/diff/420001/content/browser/web_contents/web_contents_impl.cc#newcode1853 content/browser/web_contents/web_contents_impl.cc:1853: // have a chance to create more ...

4 years, 10 months ago (2016-02-02 20:51:59 UTC) #37

nasko

Uhh, I realized I didn't send my comments : (. https://codereview.chromium.org/1542743002/diff/380001/content/browser/loader/resource_dispatcher_host_impl.h File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/1542743002/diff/380001/content/browser/loader/resource_dispatcher_host_impl.h#newcode117 ...

4 years, 10 months ago (2016-02-02 21:26:30 UTC) #38

Charlie Harrison

Thanks, Nasko! https://codereview.chromium.org/1542743002/diff/380001/content/browser/loader/resource_dispatcher_host_impl.h File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/1542743002/diff/380001/content/browser/loader/resource_dispatcher_host_impl.h#newcode117 content/browser/loader/resource_dispatcher_host_impl.h:117: // Blocks (and does not start) all ...

4 years, 10 months ago (2016-02-02 21:44:23 UTC) #39

nasko

LGTM with last couple of nits. https://codereview.chromium.org/1542743002/diff/440001/content/public/browser/resource_dispatcher_host.h File content/public/browser/resource_dispatcher_host.h (right): https://codereview.chromium.org/1542743002/diff/440001/content/public/browser/resource_dispatcher_host.h#newcode36 content/public/browser/resource_dispatcher_host.h:36: // blocked (not ...

4 years, 10 months ago (2016-02-03 18:44:00 UTC) #40

Randy Smith (Not in Mondays)

On 2016/02/03 18:44:00, nasko wrote: > LGTM with last couple of nits. > > https://codereview.chromium.org/1542743002/diff/440001/content/public/browser/resource_dispatcher_host.h ...

4 years, 10 months ago (2016-02-03 18:58:31 UTC) #41

Charlie Harrison

My take on the FromUI naming choice: My plan was for all static methods to ...

4 years, 10 months ago (2016-02-03 19:20:21 UTC) #42

Charlie Harrison

csharrison@chromium.org changed reviewers: + rdevlin.cronin@chromium.org

4 years, 10 months ago (2016-02-03 19:21:30 UTC) #43

Devlin

extensions lgtm https://codereview.chromium.org/1542743002/diff/460001/extensions/browser/app_window/app_window_contents.cc File extensions/browser/app_window/app_window_contents.cc (right): https://codereview.chromium.org/1542743002/diff/460001/extensions/browser/app_window/app_window_contents.cc#newcode92 extensions/browser/app_window/app_window_contents.cc:92: content::ResourceDispatcherHost::ResumeBlockedRequestsForFrameFromUI( Yay type checking! I had a ...

4 years, 10 months ago (2016-02-03 21:02:56 UTC) #45

Charlie Harrison

The CQ bit was checked by csharrison@chromium.org

4 years, 10 months ago (2016-02-03 21:12:00 UTC) #46

Charlie Harrison

The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org, nasko@chromium.org Link to ...

4 years, 10 months ago (2016-02-03 21:12:00 UTC) #47

commit-bot: I haz the power

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

4 years, 10 months ago (2016-02-03 21:12:59 UTC) #48

nasko

On 2016/02/03 19:20:21, csharrison wrote: > My take on the FromUI naming choice: > > ...

4 years, 10 months ago (2016-02-03 21:45:09 UTC) #49

Charlie Harrison

On 2016/02/03 21:45:09, nasko wrote: > On 2016/02/03 19:20:21, csharrison wrote: > > My take ...

4 years, 10 months ago (2016-02-03 21:48:22 UTC) #50

commit-bot: I haz the power

Description was changed from ========== Refactored blocked_loaders_map_ to key by render frame route id This ...

4 years, 10 months ago (2016-02-03 23:22:39 UTC) #52

commit-bot: I haz the power

4 years, 10 months ago (2016-02-03 23:22:41 UTC) #53

Message was sent while issue was closed.

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

Issue 1542743002: [RDHI] Refactored blocked_loaders_map_ to key by render frame route id (Closed)

Description

Patch Set 1 #

Patch Set 2 : fix unit tests #

Patch Set 3 : Add another call to RenderFrameHost::Init() #

Patch Set 4 : Add WebContentsObserver to notify frame deletion #

Patch Set 5 : fix tests #

Patch Set 6 : Fix cross site test (detached frame) for non-started request #

Patch Set 7 : ForEachFrame Notifier interface #

Patch Set 8 : just comments / minor restructuring (trybots previous) #

Patch Set 9 : Addressed Randy's comments #

Patch Set 10 : Better type safety (wrap those ints) #

Patch Set 11 : cleanup #

Patch Set 12 : Responded to nasko/rdsmith and added dependent patch set #

Patch Set 13 : cleanup #

Patch Set 14 : fix lambda return value #

Patch Set 15 : rebased on #369795 #

Patch Set 16 : merged onto #370424 #

Patch Set 17 : Replace FrameTree::ForEach with a for loop #

Patch Set 18 : Addressed comments (no architectural change) #

Patch Set 19 : break statements in switch #

Patch Set 20 : New, simpler API (default recurses frame tree) #

Patch Set 21 : Randy review #

Patch Set 22 : fix browsertest #

Patch Set 23 : Nasko review (name change + comments) #

Patch Set 24 : nasko nit #

Messages