Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1564963002:
[on hold] Remove dependency on RenderViewHost from LoadState notifications
|
Created:
4 years, 11 months ago by Charlie Harrison Modified:
3 years, 6 months ago CC:
chromium-reviews, loading-reviews_chromium.org, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org, site-isolation-reviews_chromium.org, Charlie Reis Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove dependency on RenderViewHost from LoadState notifications
This patch removes indirection from
ResourceDispatcherHostImpl -> RenderViewHost -> RenderViewHostDelegate
and instead calls the WebContents directly.
BUG=472869
Patch Set 1 #Patch Set 2 : Rebased on #368163 #Patch Set 3 : de-dupe based on frame_id on IO thread, then by WebContents* on UI thread #Patch Set 4 : Fix unittests #
Total comments: 4
Patch Set 5 : make LoadInfo CONTENT_EXPORT for tests #
Total comments: 22
Patch Set 6 : mmenke@ + nasko@ review #
Total comments: 4
Patch Set 7 : only notify navigations / uploads #Messages
Total messages: 43 (10 generated)
csharrison@chromium.org changed reviewers: + clamy@chromium.org, davidben@chromium.org
PTAL. This is part of the effort to keep render_view_ids out of ResourceDispatcherHost. Unfortunately, this CL doesn't help on the id front. We still group the load info structs by the GlobalRoutingID. We need some way of grouping them based on their WebContents. Alternatively, we could send a vector of load infos to each WebContents. The receiving WebContents could determine which is most interesting. This could be alleviated by only sending the most interesting per frame to the WebContents. WDYT?
[+site-isolation-reviews@ as FYI]
csharrison@chromium.org changed reviewers: + creis@chromium.org - davidben@chromium.org
Let's revisit this CL, in light of the fact that FrameIOData is not going to happen. Let's just send the load states by their RFID, and patch them together at the WebContents layer. Removing davidben@ and adding creis@, though this is not quite ready for review.
I did not have time to review it today, but I will try to have a look at it tomorrow.
On 2016/05/17 15:22:44, clamy wrote: > I did not have time to review it today, but I will try to have a look at it > tomorrow. This isn't quite ready for review yet, I'll probably have a patch in before tomorrow but I might not. I'll need to implement what I described in comment #5.
Alright I've updated the logic + tests to now 1. De-dupe load states by frame id 2. Send those states to the main thread 3. De-dupe those load states by their associated web contents (1) could be removed, though it would require allocating a larger map in memory (of all the loads).
mmenke@chromium.org changed reviewers: + mmenke@chromium.org
Driveby question. Clearly I just don't have enough reviews on my plate already. https://codereview.chromium.org/1564963002/diff/60001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1564963002/diff/60001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2499: contents_map[web_contents] = &load_info.second; Why construct a map, instead of just calling LoadStateChanged here?
https://codereview.chromium.org/1564963002/diff/60001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1564963002/diff/60001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2499: contents_map[web_contents] = &load_info.second; On 2016/05/17 21:25:36, mmenke wrote: > Why construct a map, instead of just calling LoadStateChanged here? We want to call LoadStateChanged with the load associated with the web contents that has progressed the most. Calling it multiple times for loads that are in different places could confuse web contents / web contents delegates that listen for these notifications. I'm not confident, but I think it could potentially cause jitter in the throbber.
https://codereview.chromium.org/1564963002/diff/60001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1564963002/diff/60001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2499: contents_map[web_contents] = &load_info.second; On 2016/05/17 21:35:28, csharrison wrote: > On 2016/05/17 21:25:36, mmenke wrote: > > Why construct a map, instead of just calling LoadStateChanged here? > > We want to call LoadStateChanged with the load associated with the web contents > that has progressed the most. Calling it multiple times for loads that are in > different places could confuse web contents / web contents delegates that listen > for these notifications. I'm not confident, but I think it could potentially > cause jitter in the throbber. Ah, I missed the LoadInfoIsMoreInteresting call. Ok, that makes sense... Well, it's a little weird that we do this exact same thing on a per=RenderFrameHost basis on the IO thread, and then do it again over on the UI thread on a WC basis, but doesn't seem like we have much choice in the matter, unless we want to pass the load state of all requests over to the UI thread, and then do one per-webcontents pass. Doesn't seem like that gets us anything, though.
https://codereview.chromium.org/1564963002/diff/60001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1564963002/diff/60001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2499: contents_map[web_contents] = &load_info.second; On 2016/05/17 21:44:04, mmenke wrote: > On 2016/05/17 21:35:28, csharrison wrote: > > On 2016/05/17 21:25:36, mmenke wrote: > > > Why construct a map, instead of just calling LoadStateChanged here? > > > > We want to call LoadStateChanged with the load associated with the web > contents > > that has progressed the most. Calling it multiple times for loads that are in > > different places could confuse web contents / web contents delegates that > listen > > for these notifications. I'm not confident, but I think it could potentially > > cause jitter in the throbber. > > Ah, I missed the LoadInfoIsMoreInteresting call. Ok, that makes sense... Well, > it's a little weird that we do this exact same thing on a per=RenderFrameHost > basis on the IO thread, and then do it again over on the UI thread on a WC > basis, but doesn't seem like we have much choice in the matter, unless we want > to pass the load state of all requests over to the UI thread, and then do one > per-webcontents pass. Doesn't seem like that gets us anything, though. Yeah it's a little awkward. I think this is better because if we send all the load states, we'd necessarily have to allocate memory to store data (including urls which can be huge) for all pending_loaders_. This way we can at least cut that down to O(url * num_frames_with_pending_loads).
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1564963002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1564963002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1564963002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1564963002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
creis@, clamy@ WDYT?
creis@chromium.org changed reviewers: + nasko@chromium.org - creis@chromium.org
Seems ok at a quick glance, but there's enough about "more interesting" loads and iterating over the map in RDHI that it's worth a closer look. I'm busy at the moment reviewing a large CL, so Nasko has offered to review this one for me.
Thanks for doing this! This seems ok to me. Just checking: this won't be working for some downloads, but I think it already doesn't work right? (And I imagine we probably don't care about sending load state notifications to the WebContents in that case...). https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.h:354: // Associates the load with a web contents. nit: empty line above this line?
A couple nits. https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2482: // load state per WebContents. Suggest moving this inside the method, or to the header - in general, function-level comments should go with the function declaration. https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2507: it.second->upload_size); Is it possible that calling LoadStateChanged on one WebContentsImpl would result in another being destroyed? Old code didn't have to worry about that, but this code does. Invoke the WC getter to avoid that. I also assume it's impossible to re-parent a RenderFrame across WebContentses (WebContenti?). https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.h:349: LoadInfo(const LoadInfo& info); Why do we need a copy constructor? Some reason the default one doesn't do the trick? Also, constructors should go before destructor, per Google style guide. https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.h:350: GURL url; Blank line between methods and members.
https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2507: it.second->upload_size); On 2016/06/01 15:19:32, mmenke (OOO May 30-31) wrote: > Is it possible that calling LoadStateChanged on one WebContentsImpl would result > in another being destroyed? Old code didn't have to worry about that, but this > code does. If it is possible with the new code, it was possible in the old one too. > Invoke the WC getter to avoid that. > > I also assume it's impossible to re-parent a RenderFrame across WebContentses > (WebContenti?). Frames cannot be reparented or moved around at all. The moment they are detached from the parent, they are destroyed. https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2542: // Group load info by frame, then pass to the UI thread where each WebContents nit: s/load info/LoadInfo/ https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.h:358: // Map from ProcessID+FrameRouteID pair to the "most interesting" LoadState. I wonder if this comment is now correct. It used to be RVH id and LoadInfo, which was 1->many mapping, since RVH id could reflect multiple frames. Now that it is a RenderFrameHost and LoadInfo, isn't it 1->1 mapping? At which point it is no longer "most interesting", but just a straight mapping. Or is it also tracking all subresource requests too, which will make it 1->many again. In general, I'd rather see "most interesting" clarified, as this is very ambiguous and not very helpful. https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_unittest.cc:941: 0, request_info[i].route_id.frame_routing_id, i + 1, Don't use 0, use MSG_ROUTING_NONE, as there is no RenderViewHost associated here. https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_unittest.cc:3597: const GlobalFrameRoutingId kId(filter_->child_id(), 0); nit: I'd use a non-zero integer as zero is not a valid routing id.
https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2507: it.second->upload_size); On 2016/06/01 16:54:00, nasko (slow) wrote: > On 2016/06/01 15:19:32, mmenke (OOO May 30-31) wrote: > > Is it possible that calling LoadStateChanged on one WebContentsImpl would > result > > in another being destroyed? Old code didn't have to worry about that, but > this > > code does. > > If it is possible with the new code, it was possible in the old one too. The way the old code was written here, though, it didn't matter with respect to *this* code, as long as the WCGetters could handle it. Now, however, we cache the WC pointers, so if one is destroyed, we have a problem. > > Invoke the WC getter to avoid that. > > > > I also assume it's impossible to re-parent a RenderFrame across WebContentses > > (WebContenti?). > > Frames cannot be reparented or moved around at all. The moment they are detached > from the parent, they are destroyed.
Okay here's the rundown on load state stuff. There's a few important things the WebContents needs to know: 1. Are we waiting for a response to update the throbber? Right now this is updated when we receive a title for the page, or when we get a load state notification LOAD_STATE_READING_RESPONSE (from any "most interesting load"). This changes the WebContent's waiting_for_response_ member. 2. If the WebContents is loading, we let the delegate know that the NavigationState has changed (this is super confusing). It seems like this happens even if no change happens (i.e. the most interesting load did not change states). I think the browser listens for this notification to update the UI. Seems like reducing the number of calls here (for when the state actually changes, or for true navigation updates) could reduce UI work. 3. Update upload information for the upload progress bar. Seems like (3) is distinct from (1) and (2), and could be split into another method (UpdateUploadProgress). This would prevent a navigation state change from interrupting an upload progress notification. In general, the "load is more interesting" check just sees if a load state is > another (which indicates if it is further along). Could we replace these checks with MAIN_FRAME only checks, and do upload notifications separately? https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2482: // load state per WebContents. On 2016/06/01 15:19:32, mmenke wrote: > Suggest moving this inside the method, or to the header - in general, > function-level comments should go with the function declaration. Moved to the header. https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2507: it.second->upload_size); On 2016/06/01 16:58:49, mmenke wrote: > On 2016/06/01 16:54:00, nasko (slow) wrote: > > On 2016/06/01 15:19:32, mmenke (OOO May 30-31) wrote: > > > Is it possible that calling LoadStateChanged on one WebContentsImpl would > > result > > > in another being destroyed? Old code didn't have to worry about that, but > > this > > > code does. > > > > If it is possible with the new code, it was possible in the old one too. > > The way the old code was written here, though, it didn't matter with respect to > *this* code, as long as the WCGetters could handle it. Now, however, we cache > the WC pointers, so if one is destroyed, we have a problem. > > > > Invoke the WC getter to avoid that. > > > > > > I also assume it's impossible to re-parent a RenderFrame across > WebContentses > > > (WebContenti?). > > > > Frames cannot be reparented or moved around at all. The moment they are > detached > > from the parent, they are destroyed. > You're right. I would say it's reasonable to assume LoadStateChanged will not delete some other WC, but I'll do the safe thing here. https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.cc:2542: // Group load info by frame, then pass to the UI thread where each WebContents On 2016/06/01 16:54:00, nasko (slow) wrote: > nit: s/load info/LoadInfo/ Done. https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.h:349: LoadInfo(const LoadInfo& info); On 2016/06/01 15:19:32, mmenke wrote: > Why do we need a copy constructor? Some reason the default one doesn't do the > trick? > > Also, constructors should go before destructor, per Google style guide. "Complex class/struct needs an explicit out-of-line copy constructor" unfortunately. This is just chromium style. Moved it before the destructor. I think the complexity comes from the Closure. https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.h:350: GURL url; On 2016/06/01 15:19:32, mmenke wrote: > Blank line between methods and members. Done. https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.h:354: // Associates the load with a web contents. On 2016/06/01 13:59:34, clamy wrote: > nit: empty line above this line? Done. https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.h:358: // Map from ProcessID+FrameRouteID pair to the "most interesting" LoadState. On 2016/06/01 16:54:00, nasko (slow) wrote: > I wonder if this comment is now correct. It used to be RVH id and LoadInfo, > which was 1->many mapping, since RVH id could reflect multiple frames. Now that > it is a RenderFrameHost and LoadInfo, isn't it 1->1 mapping? At which point it > is no longer "most interesting", but just a straight mapping. Or is it also > tracking all subresource requests too, which will make it 1->many again. > > In general, I'd rather see "most interesting" clarified, as this is very > ambiguous and not very helpful. Yeah this is still a 1:many relationship because it tracks all subresource loads. We should clarify "most interesting" though. Keeping logic as is for this PS though. https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_unittest.cc (right): https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_unittest.cc:941: 0, request_info[i].route_id.frame_routing_id, i + 1, On 2016/06/01 16:54:00, nasko (slow) wrote: > Don't use 0, use MSG_ROUTING_NONE, as there is no RenderViewHost associated > here. Done. https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_unittest.cc:3597: const GlobalFrameRoutingId kId(filter_->child_id(), 0); On 2016/06/01 16:54:00, nasko (slow) wrote: > nit: I'd use a non-zero integer as zero is not a valid routing id. Done.
https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.h:349: LoadInfo(const LoadInfo& info); On 2016/06/02 14:22:28, csharrison wrote: > On 2016/06/01 15:19:32, mmenke wrote: > > Why do we need a copy constructor? Some reason the default one doesn't do the > > trick? > > > > Also, constructors should go before destructor, per Google style guide. > > "Complex class/struct needs an explicit out-of-line copy constructor" > unfortunately. This is just chromium style. Moved it before the destructor. > > I think the complexity comes from the Closure. Can't you just use "= default;" in the cc file?
On 2016/06/02 14:22:28, csharrison wrote: > Okay here's the rundown on load state stuff. There's a few important things the > WebContents needs to know: > > 1. Are we waiting for a response to update the throbber? Right now this is > updated when we receive a title for the page, or when we get a load state > notification LOAD_STATE_READING_RESPONSE (from any "most interesting load"). > This changes the WebContent's waiting_for_response_ member. > > 2. If the WebContents is loading, we let the delegate know that the > NavigationState has changed (this is super confusing). It seems like this > happens even if no change happens (i.e. the most interesting load did not change > states). I think the browser listens for this notification to update the UI. > Seems like reducing the number of calls here (for when the state actually > changes, or for true navigation updates) could reduce UI work. > > 3. Update upload information for the upload progress bar. > > Seems like (3) is distinct from (1) and (2), and could be split into another > method (UpdateUploadProgress). This would prevent a navigation state change from > interrupting an upload progress notification. > > In general, the "load is more interesting" check just sees if a load state is > > another (which indicates if it is further along). Could we replace these checks > with MAIN_FRAME only checks, and do upload notifications separately? I thought progress bars were updated by blink? You're suggesting just using the status of the main frame request? What if the main frame is complete, and a subresource is blocked on a borked proxy, an extension, a PAC script, or something.
On 2016/06/02 15:24:45, mmenke wrote: > On 2016/06/02 14:22:28, csharrison wrote: > > Okay here's the rundown on load state stuff. There's a few important things > the > > WebContents needs to know: > > > > 1. Are we waiting for a response to update the throbber? Right now this is > > updated when we receive a title for the page, or when we get a load state > > notification LOAD_STATE_READING_RESPONSE (from any "most interesting load"). > > This changes the WebContent's waiting_for_response_ member. > > > > 2. If the WebContents is loading, we let the delegate know that the > > NavigationState has changed (this is super confusing). It seems like this > > happens even if no change happens (i.e. the most interesting load did not > change > > states). I think the browser listens for this notification to update the UI. > > Seems like reducing the number of calls here (for when the state actually > > changes, or for true navigation updates) could reduce UI work. > > > > 3. Update upload information for the upload progress bar. > > > > Seems like (3) is distinct from (1) and (2), and could be split into another > > method (UpdateUploadProgress). This would prevent a navigation state change > from > > interrupting an upload progress notification. > > > > In general, the "load is more interesting" check just sees if a load state is > > > > another (which indicates if it is further along). Could we replace these > checks > > with MAIN_FRAME only checks, and do upload notifications separately? > > I thought progress bars were updated by blink? The upload progress isn't really a "bar", it's just an update of the status text at the bottom right of the window. See https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > You're suggesting just using the status of the main frame request? What if the > main frame is complete, and a subresource is blocked on a borked proxy, an > extension, a PAC script, or something. I'll need to look through the code some more, but if the main frame has completed, then the throbber will have changed anyways. The only other case is the NavigationStateChanged callback, which I'll need to trace through all delegates to see what behavior they need.
On 2016/06/02 16:23:24, csharrison wrote: > On 2016/06/02 15:24:45, mmenke wrote: > > On 2016/06/02 14:22:28, csharrison wrote: > > > Okay here's the rundown on load state stuff. There's a few important things > > the > > > WebContents needs to know: > > > > > > 1. Are we waiting for a response to update the throbber? Right now this is > > > updated when we receive a title for the page, or when we get a load state > > > notification LOAD_STATE_READING_RESPONSE (from any "most interesting load"). > > > This changes the WebContent's waiting_for_response_ member. > > > > > > 2. If the WebContents is loading, we let the delegate know that the > > > NavigationState has changed (this is super confusing). It seems like this > > > happens even if no change happens (i.e. the most interesting load did not > > change > > > states). I think the browser listens for this notification to update the UI. > > > Seems like reducing the number of calls here (for when the state actually > > > changes, or for true navigation updates) could reduce UI work. > > > > > > 3. Update upload information for the upload progress bar. > > > > > > Seems like (3) is distinct from (1) and (2), and could be split into another > > > method (UpdateUploadProgress). This would prevent a navigation state change > > from > > > interrupting an upload progress notification. > > > > > > In general, the "load is more interesting" check just sees if a load state > is > > > > > > another (which indicates if it is further along). Could we replace these > > checks > > > with MAIN_FRAME only checks, and do upload notifications separately? > > > > I thought progress bars were updated by blink? > The upload progress isn't really a "bar", it's just an update of the status text > at the bottom right of the window. > See > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... Sorry, I thought you meant the progress bar we display on Android. > > You're suggesting just using the status of the main frame request? What if > the > > main frame is complete, and a subresource is blocked on a borked proxy, an > > extension, a PAC script, or something. > I'll need to look through the code some more, but if the main frame has > completed, then the throbber will have changed anyways. The only other case is > the NavigationStateChanged callback, which I'll need to trace through all > delegates to see what behavior they need. I thought the throbber stopped only after things like images were loaded.
On 2016/06/02 18:14:01, mmenke wrote: > On 2016/06/02 16:23:24, csharrison wrote: > > On 2016/06/02 15:24:45, mmenke wrote: > > > On 2016/06/02 14:22:28, csharrison wrote: > > > > Okay here's the rundown on load state stuff. There's a few important > things > > > the > > > > WebContents needs to know: > > > > > > > > 1. Are we waiting for a response to update the throbber? Right now this is > > > > updated when we receive a title for the page, or when we get a load state > > > > notification LOAD_STATE_READING_RESPONSE (from any "most interesting > load"). > > > > This changes the WebContent's waiting_for_response_ member. > > > > > > > > 2. If the WebContents is loading, we let the delegate know that the > > > > NavigationState has changed (this is super confusing). It seems like this > > > > happens even if no change happens (i.e. the most interesting load did not > > > change > > > > states). I think the browser listens for this notification to update the > UI. > > > > Seems like reducing the number of calls here (for when the state actually > > > > changes, or for true navigation updates) could reduce UI work. > > > > > > > > 3. Update upload information for the upload progress bar. > > > > > > > > Seems like (3) is distinct from (1) and (2), and could be split into > another > > > > method (UpdateUploadProgress). This would prevent a navigation state > change > > > from > > > > interrupting an upload progress notification. > > > > > > > > In general, the "load is more interesting" check just sees if a load state > > is > > > > > > > > another (which indicates if it is further along). Could we replace these > > > checks > > > > with MAIN_FRAME only checks, and do upload notifications separately? > > > > > > I thought progress bars were updated by blink? > > The upload progress isn't really a "bar", it's just an update of the status > text > > at the bottom right of the window. > > See > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > Sorry, I thought you meant the progress bar we display on Android. > > > > You're suggesting just using the status of the main frame request? What if > > the > > > main frame is complete, and a subresource is blocked on a borked proxy, an > > > extension, a PAC script, or something. > > I'll need to look through the code some more, but if the main frame has > > completed, then the throbber will have changed anyways. The only other case is > > the NavigationStateChanged callback, which I'll need to trace through all > > delegates to see what behavior they need. > > I thought the throbber stopped only after things like images were loaded. Yeah, I need to see where that's hooked up (Load event, maybe?). I was referring to the change between "waiting for response" and "waiting for load".
On 2016/06/02 14:22:28, csharrison wrote: > Okay here's the rundown on load state stuff. There's a few important things the > WebContents needs to know: > > 1. Are we waiting for a response to update the throbber? Right now this is > updated when we receive a title for the page, or when we get a load state > notification LOAD_STATE_READING_RESPONSE (from any "most interesting load"). > This changes the WebContent's waiting_for_response_ member. Yuck, we have both WC::LoadStateChanged and WC::LoadingStateChanged, both doing somewhat different things : (. > 2. If the WebContents is loading, we let the delegate know that the > NavigationState has changed (this is super confusing). It seems like this > happens even if no change happens (i.e. the most interesting load did not change > states). I think the browser listens for this notification to update the UI. > Seems like reducing the number of calls here (for when the state actually > changes, or for true navigation updates) could reduce UI work. > > 3. Update upload information for the upload progress bar. > > Seems like (3) is distinct from (1) and (2), and could be split into another > method (UpdateUploadProgress). This would prevent a navigation state change from > interrupting an upload progress notification. Yeah, splitting it out to be explicit will likely make it simpler to follow. Feel free to put up a separate CL for that so we can see how it looks. > In general, the "load is more interesting" check just sees if a load state is > > another (which indicates if it is further along). Could we replace these checks > with MAIN_FRAME only checks, and do upload notifications separately? Indeed it makes sense to split the navigation part from the upload one. However, I'm not sure if we can ignore SUBFRAME requests, as you can start a navigation in a subframe and it should behave the same way as though you navigated the main one. However, I think we can ignore subresources for notifying whether we have received a response or not. This will simplify the non-upload parts since we won't really care about "most interesting" load, as long as we have received any bits for the frame navigation.
On 2016/06/02 18:16:39, csharrison wrote: > On 2016/06/02 18:14:01, mmenke wrote: > > On 2016/06/02 16:23:24, csharrison wrote: > > > On 2016/06/02 15:24:45, mmenke wrote: > > > > On 2016/06/02 14:22:28, csharrison wrote: > > > > > Okay here's the rundown on load state stuff. There's a few important > > things > > > > the > > > > > WebContents needs to know: > > > > > > > > > > 1. Are we waiting for a response to update the throbber? Right now this > is > > > > > updated when we receive a title for the page, or when we get a load > state > > > > > notification LOAD_STATE_READING_RESPONSE (from any "most interesting > > load"). > > > > > This changes the WebContent's waiting_for_response_ member. > > > > > > > > > > 2. If the WebContents is loading, we let the delegate know that the > > > > > NavigationState has changed (this is super confusing). It seems like > this > > > > > happens even if no change happens (i.e. the most interesting load did > not > > > > change > > > > > states). I think the browser listens for this notification to update the > > UI. > > > > > Seems like reducing the number of calls here (for when the state > actually > > > > > changes, or for true navigation updates) could reduce UI work. > > > > > > > > > > 3. Update upload information for the upload progress bar. > > > > > > > > > > Seems like (3) is distinct from (1) and (2), and could be split into > > another > > > > > method (UpdateUploadProgress). This would prevent a navigation state > > change > > > > from > > > > > interrupting an upload progress notification. > > > > > > > > > > In general, the "load is more interesting" check just sees if a load > state > > > is > > > > > > > > > > another (which indicates if it is further along). Could we replace these > > > > checks > > > > > with MAIN_FRAME only checks, and do upload notifications separately? > > > > > > > > I thought progress bars were updated by blink? > > > The upload progress isn't really a "bar", it's just an update of the status > > text > > > at the bottom right of the window. > > > See > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/... > > > > Sorry, I thought you meant the progress bar we display on Android. > > > > > > You're suggesting just using the status of the main frame request? What > if > > > the > > > > main frame is complete, and a subresource is blocked on a borked proxy, an > > > > extension, a PAC script, or something. > > > I'll need to look through the code some more, but if the main frame has > > > completed, then the throbber will have changed anyways. The only other case > is > > > the NavigationStateChanged callback, which I'll need to trace through all > > > delegates to see what behavior they need. > > > > I thought the throbber stopped only after things like images were loaded. > > Yeah, I need to see where that's hooked up (Load event, maybe?). I was referring > to the change between "waiting for response" and "waiting for load". It is in WebContentsImpl::DidStopLoading, which calls LoadingStateChanged.
Couple of nits on the latest PS. https://codereview.chromium.org/1564963002/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1564963002/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2500: // Re-run the getter to prevent u-a-f errors if calling LoadStateChanged on Please expand u-a-f, as it isn't really common abbreviation. https://codereview.chromium.org/1564963002/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/1564963002/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.h:464: // static No need for "// static" line here.
I think I'm going to some of the proposed refactors to a follow up CL (i.e. sending upload notifications separately). The main difference in this PS is we only send data for navigation requests, or requests that are currently uploading. https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/1564963002/diff/80001/content/browser/loader/... content/browser/loader/resource_dispatcher_host_impl.h:349: LoadInfo(const LoadInfo& info); On 2016/06/02 15:20:17, mmenke wrote: > On 2016/06/02 14:22:28, csharrison wrote: > > On 2016/06/01 15:19:32, mmenke wrote: > > > Why do we need a copy constructor? Some reason the default one doesn't do > the > > > trick? > > > > > > Also, constructors should go before destructor, per Google style guide. > > > > "Complex class/struct needs an explicit out-of-line copy constructor" > > unfortunately. This is just chromium style. Moved it before the destructor. > > > > I think the complexity comes from the Closure. > > Can't you just use "= default;" in the cc file? Oh, yeah I can do that. Done. Sorry I think I misunderstood you before. https://codereview.chromium.org/1564963002/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/1564963002/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:2500: // Re-run the getter to prevent u-a-f errors if calling LoadStateChanged on On 2016/06/02 22:04:21, nasko (slow) wrote: > Please expand u-a-f, as it isn't really common abbreviation. Done. https://codereview.chromium.org/1564963002/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.h (right): https://codereview.chromium.org/1564963002/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.h:464: // static On 2016/06/02 22:04:21, nasko (slow) wrote: > No need for "// static" line here. Done.
Didn't do a full review on this, so not signing off, but I consider my comments all addressed.
nasko@, can you take another look? I'm deferring the more serious refactoring because initial prototypes didn't look great.
Description was changed from ========== Remove dependency on RenderViewHost from LoadState notifications This patch removes indirection from ResourceDispatcherHostImpl -> RenderViewHost -> RenderViewHostDelegate and instead calls the WebContents directly. BUG=472869 ========== to ========== Remove dependency on RenderViewHost from LoadState notifications This patch removes indirection from ResourceDispatcherHostImpl -> RenderViewHost -> RenderViewHostDelegate and instead calls the WebContents directly. BUG=472869 ==========
On 2016/06/17 10:45:50, Charlie Harrison wrote: > nasko@, can you take another look? I'm deferring the more serious refactoring > because initial prototypes didn't look great. Removing myself from this CL. Feel free to add me back if you pick it up again.
mmenke@chromium.org changed reviewers: - mmenke@chromium.org |
