Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
925623002:
Refactor the loading tracking logic in WebContentsImpl. (Closed)
|
Created:
5 years, 10 months ago by Fabrice (no longer in Chrome) Modified:
5 years, 9 months ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor the loading tracking logic in WebContentsImpl.
This removes the loading_progresses_ map and the
loading_frames_in_progress counter in WebContentsImpl.
Instead, tracking of these is done at the RFHI level.
BUG=429399
Committed: https://crrev.com/bda82dd8f2286787212cb92c694a999f08834c7b
Cr-Commit-Position: refs/heads/master@{#319267}
Patch Set 1 #
Total comments: 36
Patch Set 2 : Review comments #Patch Set 3 : Fix compile #
Total comments: 6
Patch Set 4 : WIP (Fix tests) #
Total comments: 31
Patch Set 5 : WIP, just to run tests #
Total comments: 8
Patch Set 6 : Review comments + track per RFH #
Total comments: 9
Patch Set 7 : Review comments + fix #
Total comments: 8
Patch Set 8 : Review comments + add test #
Total comments: 6
Patch Set 9 : Rebase #
Total comments: 12
Patch Set 10 : Review comments #
Total comments: 26
Patch Set 11 : Review comments. #
Total comments: 12
Patch Set 12 : Review comments + rebase #
Total comments: 15
Patch Set 13 : Review comments #
Total comments: 16
Patch Set 14 : Review comments #Patch Set 15 : Rebase #Patch Set 16 : Comments consistency. #
Total comments: 6
Patch Set 17 : Nitses. #Messages
Total messages: 50 (5 generated)
fdegans@chromium.org changed reviewers: + carlosk@chromium.org, clamy@chromium.org
PTAL
This is the first part of the Did{Start|Stop}Loading refactor, as discussed.
fdegans@chromium.org changed reviewers: + nasko@chromium.org
Just a couple of nits on a quick glimpse. The bigger issue is that this CL needs a test to ensure the new code behaves correctly. https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:2901: // TODO(japhet): This test should not exist, but the pdf plugin sometimes nit: s/test/check/ https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.h:969: // Returns true if one of the nodes in the frame_tree_ is loading, nit: "at least one of", |frame_tree_|
creis@chromium.org changed reviewers: + creis@chromium.org
Thanks for taking this! I'm hopeful that this will fix some of the infinite spinner cases, so it will have a big impact for users. A few nits and questions in addition to Nasko's. https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... File content/browser/web_contents/web_contents_impl.cc (left): https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:2921: loading_weak_factory_.InvalidateWeakPtrs(); Why is this safe to remove? Is the factory still needed? https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:168: // Helper function for retrieving the total loading progress nit: Please try to wrap lines closer to 80 chars for consistency with other code. https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:170: bool CollectProgress(double* progress, int* frame_count, FrameTreeNode* node) { nit: This name is a bit ambiguous. Maybe CollectLoadProgress? https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:180: return false; nit: Put a comment here saying that this aborts the traversal, since otherwise it looks a bit odd to return false here. https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:2890: // Load stopped while the load was still being tracked. This comment doesn't make sense if it's not within the if. It's possible the load isn't still being tracked. It's also not clear to me why the if was removed. How is that case handled now? https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.h:971: bool IsTreeLoading(); nit: IsLoading might be clearer.
Also, for what it's worth, I had been experimenting with a test for the case that I thought was broken, but I couldn't get it to work at the time. Not sure if that code is useful or should just be thrown away, but here's the draft if you're interested: https://codereview.chromium.org/853083003/
Thanks! A few comments from my part. https://codereview.chromium.org/925623002/diff/1/content/browser/frame_host/f... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/925623002/diff/1/content/browser/frame_host/f... content/browser/frame_host/frame_tree_node.h:118: double get_loading_progress() const { return loading_progress_; } s/get_loading_progress()/loading_progress() nit: Add an empty line above. https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... File content/browser/web_contents/web_contents_impl.cc (left): https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:3411: loading_progresses_.clear(); Is it okay to not reset the loading progress of each FrameTreeNode here? (as an equivalent of resetting the map).
Sorry for taking so long. https://codereview.chromium.org/925623002/diff/1/content/browser/frame_host/f... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/925623002/diff/1/content/browser/frame_host/f... content/browser/frame_host/frame_tree_node.h:173: bool is_loading_; Does the tracking of the loading progress happens in sync with the loading flag being set? If so, is it possible to eliminate the |bool| value and track loading only with the |double| (for instance is_loading() could be implemented as |return loading_progress_ >= 1.0d|)? This would avoid inconsistent states between them. https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:168: // Helper function for retrieving the total loading progress On 2015/02/13 01:01:51, Charlie Reis wrote: > nit: Please try to wrap lines closer to 80 chars for consistency with other > code. Note: git cl format does *not* do a good job when dealing with comments. https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:176: // Helper function to check if a node is loading. I'd suggest: Used with FrameTree::ForEach() to check it at least one node is loading. And something in these lines for the previous method. https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:2907: if (!IsTreeLoading()) Suggestion: IsTreeLoading could return the number of loading nodes. This maps seamlessly to the current |bool| and would allow you to not have to call it twice here. https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:3395: if (frame_count == 0) With your implementation it seems that this if would never be triggered. The previous implementation seemed to have in the map only the nodes that were loading whereas your CollectProgress function will return the tree node count (which is always >= 1). It seems this would also affect the calculated |progress| value but I'd think that was not really important and even that the new calculation would be "better". https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.h:970: // false otherwise. nit: the otherwise case here seems redundant.
Thanks a lot for the reviews everyone! Second patch set is WIP and I haven't integrated every comment yet. Mostly, I want to know how the try bots react to the new DCHECK in DidStopLoading. https://codereview.chromium.org/925623002/diff/1/content/browser/frame_host/f... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/925623002/diff/1/content/browser/frame_host/f... content/browser/frame_host/frame_tree_node.h:118: double get_loading_progress() const { return loading_progress_; } On 2015/02/13 12:13:43, clamy wrote: > s/get_loading_progress()/loading_progress() > > nit: Add an empty line above. Done. https://codereview.chromium.org/925623002/diff/1/content/browser/frame_host/f... content/browser/frame_host/frame_tree_node.h:173: bool is_loading_; On 2015/02/13 15:05:41, carlosk wrote: > Does the tracking of the loading progress happens in sync with the loading flag > being set? If so, is it possible to eliminate the |bool| value and track loading > only with the |double| (for instance is_loading() could be implemented as > |return loading_progress_ >= 1.0d|)? This would avoid inconsistent states > between them. This sounded good until I actually tried it and it caused crashes and what not. Another issue is that it is possible that due to rounding errors, we set an intermediate load progress to 1.0 while the content is not actually done loading. https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... File content/browser/web_contents/web_contents_impl.cc (left): https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:2921: loading_weak_factory_.InvalidateWeakPtrs(); On 2015/02/13 01:01:51, Charlie Reis wrote: > Why is this safe to remove? Is the factory still needed? Because I fail to merge my local branches. Fixed, sorry and thanks! https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:168: // Helper function for retrieving the total loading progress On 2015/02/13 01:01:51, Charlie Reis wrote: > nit: Please try to wrap lines closer to 80 chars for consistency with other > code. Done. https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:170: bool CollectProgress(double* progress, int* frame_count, FrameTreeNode* node) { On 2015/02/13 01:01:51, Charlie Reis wrote: > nit: This name is a bit ambiguous. Maybe CollectLoadProgress? Done. https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:176: // Helper function to check if a node is loading. On 2015/02/13 15:05:41, carlosk wrote: > I'd suggest: Used with FrameTree::ForEach() to check it at least one node is > loading. > And something in these lines for the previous method. That's exactly what is being done. These are the functions called by ForEach on each node. https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:180: return false; On 2015/02/13 01:01:51, Charlie Reis wrote: > nit: Put a comment here saying that this aborts the traversal, since otherwise > it looks a bit odd to return false here. Done. https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:2890: // Load stopped while the load was still being tracked. On 2015/02/13 01:01:51, Charlie Reis wrote: > This comment doesn't make sense if it's not within the if. It's possible the > load isn't still being tracked. > > It's also not clear to me why the if was removed. How is that case handled now? My understanding of this test is that it really should have been a DCHECK all along but I am not certain. I am replacing it with just that right now. It appears the pdf plugin issue has been fixed some time ago too. https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:2901: // TODO(japhet): This test should not exist, but the pdf plugin sometimes On 2015/02/13 00:16:29, nasko wrote: > nit: s/test/check/ Done. https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:2907: if (!IsTreeLoading()) On 2015/02/13 15:05:41, carlosk wrote: > Suggestion: IsTreeLoading could return the number of loading nodes. This maps > seamlessly to the current |bool| and would allow you to not have to call it > twice here. This is no longer necessary since I am removing the above test. https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.h:969: // Returns true if one of the nodes in the frame_tree_ is loading, On 2015/02/13 00:16:29, nasko wrote: > nit: "at least one of", |frame_tree_| Done. https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.h:971: bool IsTreeLoading(); On 2015/02/13 01:01:51, Charlie Reis wrote: > nit: IsLoading might be clearer. There already is an IsLoading in WebContents implementation. I renamed it to IsFrameTreeLoading instead.
One extra explanation. https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:176: // Helper function to check if a node is loading. On 2015/02/13 18:04:25, Fabrice wrote: > On 2015/02/13 15:05:41, carlosk wrote: > > I'd suggest: Used with FrameTree::ForEach() to check it at least one node is > > loading. > > And something in these lines for the previous method. > That's exactly what is being done. These are the functions called by ForEach on > each node. I was suggesting a new way to phrase your comment. :)
Thanks! A few comments on the new patchset. https://codereview.chromium.org/925623002/diff/1/content/browser/frame_host/f... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/925623002/diff/1/content/browser/frame_host/f... content/browser/frame_host/frame_tree_node.h:118: double get_loading_progress() const { return loading_progress_; } On 2015/02/13 18:04:24, Fabrice wrote: > On 2015/02/13 12:13:43, clamy wrote: > > s/get_loading_progress()/loading_progress() > > > > nit: Add an empty line above. > Done. I don't see the modification to the accessor name in the new patch set (see the style guide here: http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names) https://codereview.chromium.org/925623002/diff/40001/content/browser/frame_ho... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/925623002/diff/40001/content/browser/frame_ho... content/browser/frame_host/frame_tree_node.h:28: // The minimum progress vlaue matches what Blink's ProgressTracker has nit: s/vlaue/value Also I don't think that a line return is necessary here. https://codereview.chromium.org/925623002/diff/40001/content/browser/frame_ho... content/browser/frame_host/frame_tree_node.h:30: const double kNotStartedLoadingProgress = 0.0; So I've found an example of const values in the code at https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... . I think we should try to match that here. https://codereview.chromium.org/925623002/diff/40001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/925623002/diff/40001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:2936: if (loading_weak_factory_.HasWeakPtrs()) Why do you return if the loading factory has a weak pointer?
New WIP patch, should fix issues with currently failing tests. TODO: add a new test, make sure the reset logic is still correct, check the weak pointer factory is still needed. https://codereview.chromium.org/925623002/diff/1/content/browser/frame_host/f... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/925623002/diff/1/content/browser/frame_host/f... content/browser/frame_host/frame_tree_node.h:118: double get_loading_progress() const { return loading_progress_; } On 2015/02/16 14:20:59, clamy wrote: > On 2015/02/13 18:04:24, Fabrice wrote: > > On 2015/02/13 12:13:43, clamy wrote: > > > s/get_loading_progress()/loading_progress() > > > > > > nit: Add an empty line above. > > Done. > > I don't see the modification to the accessor name in the new patch set (see the > style guide here: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names) Done. https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:176: // Helper function to check if a node is loading. On 2015/02/16 11:40:21, carlosk wrote: > On 2015/02/13 18:04:25, Fabrice wrote: > > On 2015/02/13 15:05:41, carlosk wrote: > > > I'd suggest: Used with FrameTree::ForEach() to check it at least one node is > > > loading. > > > And something in these lines for the previous method. > > That's exactly what is being done. These are the functions called by ForEach > on > > each node. > > I was suggesting a new way to phrase your comment. :) Done. https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:3395: if (frame_count == 0) On 2015/02/13 15:05:41, carlosk wrote: > With your implementation it seems that this if would never be triggered. The > previous implementation seemed to have in the map only the nodes that were > loading whereas your CollectProgress function will return the tree node count > (which is always >= 1). > > It seems this would also affect the calculated |progress| value but I'd think > that was not really important and even that the new calculation would be > "better". Done. But will need to double-check if we're doing the right thing. https://codereview.chromium.org/925623002/diff/40001/content/browser/frame_ho... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/925623002/diff/40001/content/browser/frame_ho... content/browser/frame_host/frame_tree_node.h:28: // The minimum progress vlaue matches what Blink's ProgressTracker has On 2015/02/16 14:20:59, clamy wrote: > nit: s/vlaue/value > Also I don't think that a line return is necessary here. Done. https://codereview.chromium.org/925623002/diff/40001/content/browser/frame_ho... content/browser/frame_host/frame_tree_node.h:30: const double kNotStartedLoadingProgress = 0.0; On 2015/02/16 14:20:59, clamy wrote: > So I've found an example of const values in the code at > https://code.google.com/p/chromium/codesearch#chromium/src/content/browser/fr... > . I think we should try to match that here. Done.
Thanks! A few more comments. On a side note, it is harder to track what is happening with the review if you do not answer all comments from the previous round of reviews when you upload a new patchset :). https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (left): https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:3414: loading_progresses_.clear(); Is it okay to not reset the load progress for each of the FrameTreeNodes here (as an equivalent of resetting the map)? https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:2857: rfh->frame_tree_node()->set_loading_progress( It seems a bit weird to have a call to ResetLoadProgessState and then reset the load progress again on the frame tree node. Would it be possible to have it in ResetLoadProgressState? https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:2897: if (rfh->frame_tree_node()->current_frame_host() != rfh) We want to remove mention of the RenderFrameHost from the DidStart/StopLoading functions in favor of the FrameTreeNode. Additionally, the rfh you get here is the one that sent the IPC. However, we want to merge this IPC handler with the DidStopLoading function, so this would completely break.
Thanks for the updates! A few additional comments and possible bugs below. https://codereview.chromium.org/925623002/diff/60001/content/browser/frame_ho... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/925623002/diff/60001/content/browser/frame_ho... content/browser/frame_host/frame_tree_node.cc:19: double FrameTreeNode::kNotStartedLoadingProgress = 0.0; const double https://codereview.chromium.org/925623002/diff/60001/content/browser/frame_ho... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/925623002/diff/60001/content/browser/frame_ho... content/browser/frame_host/frame_tree_node.h:33: // These values indicate the loading progress status. The minimum progress Hmm, I'm not thrilled about having these as public constants on FrameTreeNode, since this class isn't primarily about loading. I'm not sure I have a better location to suggest, so maybe we can leave them here for now. https://codereview.chromium.org/925623002/diff/60001/content/browser/frame_ho... content/browser/frame_host/frame_tree_node.h:36: static double kNotStartedLoadingProgress; static const double Also, it would be clearer for LoadingProgress to be the prefix rather than suffix (e.g., kLoadingProgressNotStarted). https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (left): https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:3414: loading_progresses_.clear(); On 2015/02/20 14:00:24, clamy wrote: > Is it okay to not reset the load progress for each of the FrameTreeNodes here > (as an equivalent of resetting the map)? No. We should be resetting each node in the tree here. https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:164: // Helper function for retrieving the total loading progress and number of nit: Helper function used with FrameTree::ForEach() for... https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:178: // There is at least one node loading, abort traversal. nit: This is a run-on sentence. Use a semicolon or "loading, so abort" https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:2858: FrameTreeNode::kNotStartedLoadingProgress); It looks wrong to me to have this here at all. We might return early on line 2874, which means we won't set it to kMinimumLoadingProgress even though we're loading. https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:2896: // StopLoading was called for an older navigation, ignore. Same nit about run-on sentence. https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:2897: if (rfh->frame_tree_node()->current_frame_host() != rfh) On 2015/02/20 14:00:24, clamy wrote: > We want to remove mention of the RenderFrameHost from the DidStart/StopLoading > functions in favor of the FrameTreeNode. > > Additionally, the rfh you get here is the one that sent the IPC. However, we > want to merge this IPC handler with the DidStopLoading function, so this would > completely break. Let's not worry about the RFH->FTN switch or OnDidStop + DidStop merge just yet. I'd like to make sure we get something working for this case first. That said, we don't have a similar check in OnDidStartLoading, so I'm not sure why this is needed here. https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:3410: progress /= frame_count; Hmm, it's a little subtle to assume that we will always have at least one frame in the tree, even though that should be true. We should put a DCHECK_GT(frame_count, 0) here to show that's our expectation. (A DCHECK should be sufficient, since we'll crash either way in release builds if we get it wrong.) https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.h:972: bool IsFrameTreeLoading(); Can you put this with the other loading functions (around line 900)?
Just a couple of comments to add. https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (left): https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:3401: return; You *might* be able replace this if-check with |(progress == 0)| as this *seems* to reflect what this early return used to do within the new implementation context (meaning no nodes are loading). https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:3414: return; This is a minor/usability issue and should be sorted out later. This early return here stops the progress bar to go back in time which might be good from a usability PoV. But it might also affect a lot the "precision" or our loading tracking as this function is called very early in the loading process. Imagine this case: we're loading a document with multiple iframes and a simple one starts early and reports huge progress quickly before other iframes had a chance to be added (*). Progress got to, say, 90% and it will not move even though the others, more complex iframes will only later begin to report their progress. Am I missing something here or this sounds correct? (*) If all iframes of a document are all added at once this is not an issue. But I'm guessing that's not the case.
Few comments. https://codereview.chromium.org/925623002/diff/80001/content/browser/frame_ho... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/925623002/diff/80001/content/browser/frame_ho... content/browser/frame_host/frame_tree_node.h:191: // Double value representing the frame's completion progress (from 0 to 1). nit: s/completion/loading/ https://codereview.chromium.org/925623002/diff/80001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/925623002/diff/80001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:163: // Helper function for retrieving the total loading progress and number of nit: I like the "used with FrameTree::ForEach()" part of the comment in the function below. It will be nice to add it here too. https://codereview.chromium.org/925623002/diff/80001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:2852: if (rfh->frame_tree_node()->pending_frame_host() != rfh) { Pending RFH has a very precise meaning and it doesn't match what this code is doing. Maybe rename it to "loading_frame_host"? https://codereview.chromium.org/925623002/diff/80001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:2910: // This method should never be called when the frame is not loading. A DCHECK is warranted every time we have a "should never ..."
Thanks for all the comments! Following offline discussion with nasko@, I have moved the loading and loading progress tracking to the RFH. This greatly simplifies the code in WebContentsImpl and will make refactoring the IPC to RFHI much simpler. Answers for your pending questions from the previous patch sets inline, apologies for the delay. PTA(nother)L https://codereview.chromium.org/925623002/diff/40001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/925623002/diff/40001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:2936: if (loading_weak_factory_.HasWeakPtrs()) On 2015/02/16 14:20:59, clamy wrote: > Why do you return if the loading factory has a weak pointer? The factory is used to track whether there is already a delayed task calling SendLoadProgressChange. So we exit early here because there is no need to send the same message a second time. https://codereview.chromium.org/925623002/diff/60001/content/browser/frame_ho... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/925623002/diff/60001/content/browser/frame_ho... content/browser/frame_host/frame_tree_node.cc:19: double FrameTreeNode::kNotStartedLoadingProgress = 0.0; On 2015/02/20 23:42:12, Charlie Reis wrote: > const double Done. https://codereview.chromium.org/925623002/diff/60001/content/browser/frame_ho... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/925623002/diff/60001/content/browser/frame_ho... content/browser/frame_host/frame_tree_node.h:36: static double kNotStartedLoadingProgress; On 2015/02/20 23:42:12, Charlie Reis wrote: > static const double > > Also, it would be clearer for LoadingProgress to be the prefix rather than > suffix (e.g., kLoadingProgressNotStarted). Done. https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (left): https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:3414: loading_progresses_.clear(); On 2015/02/20 23:42:12, Charlie Reis wrote: > On 2015/02/20 14:00:24, clamy wrote: > > Is it okay to not reset the load progress for each of the FrameTreeNodes here > > (as an equivalent of resetting the map)? > > No. We should be resetting each node in the tree here. I do not think we should actually, because this method is called in multiple places. From what I can see, this is merely a "helper" method to reset the parts of the loading progress tracking logic done in WebContentsImpl. https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:164: // Helper function for retrieving the total loading progress and number of On 2015/02/20 23:42:12, Charlie Reis wrote: > nit: Helper function used with FrameTree::ForEach() for... Done. https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:178: // There is at least one node loading, abort traversal. On 2015/02/20 23:42:12, Charlie Reis wrote: > nit: This is a run-on sentence. Use a semicolon or "loading, so abort" Done. https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:2896: // StopLoading was called for an older navigation, ignore. On 2015/02/20 23:42:12, Charlie Reis wrote: > Same nit about run-on sentence. Done. https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:3410: progress /= frame_count; On 2015/02/20 23:42:12, Charlie Reis wrote: > Hmm, it's a little subtle to assume that we will always have at least one frame > in the tree, even though that should be true. > > We should put a DCHECK_GT(frame_count, 0) here to show that's our expectation. > (A DCHECK should be sufficient, since we'll crash either way in release builds > if we get it wrong.) Done. https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.h:972: bool IsFrameTreeLoading(); On 2015/02/20 23:42:12, Charlie Reis wrote: > Can you put this with the other loading functions (around line 900)? Done. https://codereview.chromium.org/925623002/diff/80001/content/browser/frame_ho... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/925623002/diff/80001/content/browser/frame_ho... content/browser/frame_host/frame_tree_node.h:191: // Double value representing the frame's completion progress (from 0 to 1). On 2015/02/23 16:54:43, nasko wrote: > nit: s/completion/loading/ Done. https://codereview.chromium.org/925623002/diff/80001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/925623002/diff/80001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:163: // Helper function for retrieving the total loading progress and number of On 2015/02/23 16:54:43, nasko wrote: > nit: I like the "used with FrameTree::ForEach()" part of the comment in the > function below. It will be nice to add it here too. Done. https://codereview.chromium.org/925623002/diff/80001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:2852: if (rfh->frame_tree_node()->pending_frame_host() != rfh) { On 2015/02/23 16:54:43, nasko wrote: > Pending RFH has a very precise meaning and it doesn't match what this code is > doing. Maybe rename it to "loading_frame_host"? Done. https://codereview.chromium.org/925623002/diff/80001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:2910: // This method should never be called when the frame is not loading. On 2015/02/23 16:54:43, nasko wrote: > A DCHECK is warranted every time we have a "should never ..." This got shuffled around, the comment is at the proper line again. Thanks.
https://codereview.chromium.org/925623002/diff/100001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/925623002/diff/100001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:125: return current_frame_host->is_loading() && pending_frame_host->is_loading(); ... s/&&/||/ I find it worrisome that none of our tests seem too bothered by it.
Hmm, why were loading and load progress moved from FrameTreeNode to RenderFrameHost? That seems confusing to me. A single load in a frame can outlive multiple RenderFrameHosts, so it seems like it should be tracked by FrameTreeNode. It's much more complicated to try to keep track of current and pending RenderFrameHosts and ensure that the exposed notion of load progress is monotonically increasing. I can chat with Nasko about it on Tuesday, but so far I liked the previous patch better. https://codereview.chromium.org/925623002/diff/100001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/925623002/diff/100001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:139: pending_frame_host->loading_progress()) / 2; This is not correct. Why would the overall load progress be the average of two different frames? For example, we make an effort to ensure that load progress never goes backwards, and this would fail that if a pending RFH were introduced in the middle of a load (e.g., for a redirect). https://codereview.chromium.org/925623002/diff/100001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/925623002/diff/100001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.h:113: // Returns true if any of the current or pending RenderFrameHost from the if either the current or pending ("any" implies there might be more than just one current and one pending, which isn't possible.) https://codereview.chromium.org/925623002/diff/100001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.h:117: // Returns the average loading progress of the RenderFrameHostImpl from the of the current RenderFrameHost (We should be consistent with the comment above.) That said, I find this confusing. I don't understand why we've moved loading and load progress to RenderFrameHost, because those are concepts that can outlive a single RenderFrameHost.
New patch, answering more older comments below. On 2015/02/24 04:55:00, Charlie Reis wrote: > Hmm, why were loading and load progress moved from FrameTreeNode to > RenderFrameHost? That seems confusing to me. A single load in a frame can > outlive multiple RenderFrameHosts, so it seems like it should be tracked by > FrameTreeNode. It's much more complicated to try to keep track of current and > pending RenderFrameHosts and ensure that the exposed notion of load progress is > monotonically increasing. > > I can chat with Nasko about it on Tuesday, but so far I liked the previous patch > better. We had a pretty long discussion with Nasko on Monday about this. Essentially, the issue was, do we track the current RFH, the pending RFH, the latest RFH, or both? We ended up deciding to tack both as none of the other options would be fully accurate. In fact I have noticed issues in our current implementation where the WCI would consider it has stopped loading too early, as a pending RFH was still loading. I think the underlying issue is that our test coverage for tracking the loading logic is lacking. From debugging, I now have a pretty good idea of what we should test and am going to add what I think is missing. https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... File content/browser/web_contents/web_contents_impl.cc (left): https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.cc:3411: loading_progresses_.clear(); On 2015/02/13 12:13:43, clamy wrote: > Is it okay to not reset the loading progress of each FrameTreeNode here? (as an > equivalent of resetting the map). Answered in patch set 4. https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.h:970: // false otherwise. On 2015/02/13 15:05:41, carlosk wrote: > nit: the otherwise case here seems redundant. Not sure about the style, but there are some others like it in the codebase: https://code.google.com/p/chromium/codesearch#search/&q=%22false%20otherwise%... https://codereview.chromium.org/925623002/diff/60001/content/browser/frame_ho... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/925623002/diff/60001/content/browser/frame_ho... content/browser/frame_host/frame_tree_node.h:33: // These values indicate the loading progress status. The minimum progress On 2015/02/20 23:42:12, Charlie Reis wrote: > Hmm, I'm not thrilled about having these as public constants on FrameTreeNode, > since this class isn't primarily about loading. I'm not sure I have a better > location to suggest, so maybe we can leave them here for now. We should be able to move these out of the class once the IPCs have been moved to RFHI. https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (left): https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:3401: return; On 2015/02/23 10:41:15, carlosk wrote: > You *might* be able replace this if-check with |(progress == 0)| as this *seems* > to reflect what this early return used to do within the new implementation > context (meaning no nodes are loading). The loading progress tracking that is done here is inaccurate at best, it is an estimate. This new way is no better nor worse. I do not think this test is necessary anyway because the test on line 3405 will return early if |(progress == 0.0)| anyway. https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:2857: rfh->frame_tree_node()->set_loading_progress( On 2015/02/20 14:00:24, clamy wrote: > It seems a bit weird to have a call to ResetLoadProgessState and then reset the > load progress again on the frame tree node. Would it be possible to have it in > ResetLoadProgressState? Answered in another comment in patch set 4, ResetLoadProgressState only resets the loading progress tracking done in WebContentsImpl, so this is correct. https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:2858: FrameTreeNode::kNotStartedLoadingProgress); On 2015/02/20 23:42:12, Charlie Reis wrote: > It looks wrong to me to have this here at all. We might return early on line > 2874, which means we won't set it to kMinimumLoadingProgress even though we're > loading. Latter patch sets no longer return early, so it is no longer relevant. https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:2897: if (rfh->frame_tree_node()->current_frame_host() != rfh) On 2015/02/20 23:42:12, Charlie Reis wrote: > On 2015/02/20 14:00:24, clamy wrote: > > We want to remove mention of the RenderFrameHost from the DidStart/StopLoading > > functions in favor of the FrameTreeNode. > > > > Additionally, the rfh you get here is the one that sent the IPC. However, we > > want to merge this IPC handler with the DidStopLoading function, so this would > > completely break. > > Let's not worry about the RFH->FTN switch or OnDidStop + DidStop merge just yet. > I'd like to make sure we get something working for this case first. > > That said, we don't have a similar check in OnDidStartLoading, so I'm not sure > why this is needed here. This is no longer relevant as we now track the loading per-frame. https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:3414: return; On 2015/02/23 10:41:15, carlosk wrote: > This is a minor/usability issue and should be sorted out later. > > This early return here stops the progress bar to go back in time which might be > good from a usability PoV. But it might also affect a lot the "precision" or our > loading tracking as this function is called very early in the loading process. > > Imagine this case: we're loading a document with multiple iframes and a simple > one starts early and reports huge progress quickly before other iframes had a > chance to be added (*). Progress got to, say, 90% and it will not move even > though the others, more complex iframes will only later begin to report their > progress. > > Am I missing something here or this sounds correct? > > (*) If all iframes of a document are all added at once this is not an issue. But > I'm guessing that's not the case. I did not want to change the logic in here so I let it to be the same way as before. I am going to guess that in most cases iframes are added early enough that it is not going to be a problem. And I think that having a loading progress tracker that goes backwards would be confusing for users. The way we are tracking it right now does have the side effect to favor the "main" frame progression, but I think this is actually a good thing as this is probably the load progress most users will care about. https://codereview.chromium.org/925623002/diff/100001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/925623002/diff/100001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:125: return current_frame_host->is_loading() && pending_frame_host->is_loading(); On 2015/02/23 20:48:45, Fabrice wrote: > ... s/&&/||/ > I find it worrisome that none of our tests seem too bothered by it. Done. I will add a test for this case. https://codereview.chromium.org/925623002/diff/100001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:139: pending_frame_host->loading_progress()) / 2; On 2015/02/24 04:55:00, Charlie Reis wrote: > This is not correct. Why would the overall load progress be the average of two > different frames? For example, we make an effort to ensure that load progress > never goes backwards, and this would fail that if a pending RFH were introduced > in the middle of a load (e.g., for a redirect). The loading progress can in fact go backwards but it is simply ignored. The way we track this has always been an estimate anyway so I am not sure why it would be a problem now. https://codereview.chromium.org/925623002/diff/100001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/925623002/diff/100001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.h:113: // Returns true if any of the current or pending RenderFrameHost from the On 2015/02/24 04:55:00, Charlie Reis wrote: > if either the current or pending > > ("any" implies there might be more than just one current and one pending, which > isn't possible.) Done. https://codereview.chromium.org/925623002/diff/100001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.h:117: // Returns the average loading progress of the RenderFrameHostImpl from the On 2015/02/24 04:55:00, Charlie Reis wrote: > of the current RenderFrameHost > > (We should be consistent with the comment above.) > > That said, I find this confusing. I don't understand why we've moved loading > and load progress to RenderFrameHost, because those are concepts that can > outlive a single RenderFrameHost. Fixed the wording but I am keeping the average loading for now.
Thanks! Please find a few comments below. @creis: we discussed that with Nasko yesterday. The issue here is the one with the WebNavigation API tests where there are two concurrent navigations in the same node and one commits without canceling the other (due to it not being user-initiated). If we use only a boolean in FTN, then the first navigation can stop while the other one has not committed yet, making the FTN believe that it stopped loading when it has not. Besides this, I am quite uncertain of what is happening/needs to happen in WebContentsImpl::DidStartLoading with the reset of the load progress. I think we should make clear what we want to achieve here. From what I understand, currently a new main frame load will trigger a reset of the load status of all subframes. In particular if a new cross-site navigation gets canceled before it commits, then it would seem that no node in the FrameTree is loading, even though the subframes have not been destroyed and may still be loading. At the same time, it makes more sense from a user point of view to consider the load status of the latest navigation (and we don't want to mix the status of the new main frame load and the current subframe loads). Therefore, I wonder if instead of resetting the subframe status upon the start of navigation we should not be ignoring them in our computation of progress status while there is an ongoing main frame navigation that has not committed yet. https://codereview.chromium.org/925623002/diff/120001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/925623002/diff/120001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:139: pending_frame_host->loading_progress()) / 2; Maybe use the max of the two rather than the average? https://codereview.chromium.org/925623002/diff/120001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/925623002/diff/120001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:216: bool is_loading() const { return is_loading_; } nit: add an empty line above (same below). https://codereview.chromium.org/925623002/diff/120001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:683: // Boolean value indicating whether this RFH in the process of loading a "is in the process..."
On 2015/02/24 13:54:55, clamy wrote: > Thanks! Please find a few comments below. > @creis: we discussed that with Nasko yesterday. The issue here is the one with > the WebNavigation API tests where there are two concurrent navigations in the > same node and one commits without canceling the other (due to it not being > user-initiated). If we use only a boolean in FTN, then the first navigation can > stop while the other one has not committed yet, making the FTN believe that it > stopped loading when it has not. Thanks. Nasko also clarified the relevant case for me (i.e., the current RFH can commit without canceling the pending RFH). Given that, I'm ok with tracking the loading state on the RFH internally and exposing it to others via FTN. > Besides this, I am quite uncertain of what is happening/needs to happen in > WebContentsImpl::DidStartLoading with the reset of the load progress. I think we > should make clear what we want to achieve here. > > From what I understand, currently a new main frame load will trigger a reset of > the load status of all subframes. In particular if a new cross-site navigation > gets canceled before it commits, then it would seem that no node in the > FrameTree is loading, even though the subframes have not been destroyed and may > still be loading. At the same time, it makes more sense from a user point of > view to consider the load status of the latest navigation (and we don't want to > mix the status of the new main frame load and the current subframe loads). > Therefore, I wonder if instead of resetting the subframe status upon the start > of navigation we should not be ignoring them in our computation of progress > status while there is an ongoing main frame navigation that has not committed > yet. You're referring to this code, right? https://codereview.chromium.org/819673002 I'm definitely open to handling that in a more principled way. https://codereview.chromium.org/925623002/diff/100001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/925623002/diff/100001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:139: pending_frame_host->loading_progress()) / 2; On 2015/02/24 13:14:47, Fabrice wrote: > On 2015/02/24 04:55:00, Charlie Reis wrote: > > This is not correct. Why would the overall load progress be the average of > two > > different frames? For example, we make an effort to ensure that load progress > > never goes backwards, and this would fail that if a pending RFH were > introduced > > in the middle of a load (e.g., for a redirect). > > The loading progress can in fact go backwards but it is simply ignored. The way > we track this has always been an estimate anyway so I am not sure why it would > be a problem now. To be clear: it's important for UX that the load progress never goes backwards, but I agree that WebContentsImpl::SendLoadProgressChanged() enforces that. https://codereview.chromium.org/925623002/diff/120001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/925623002/diff/120001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:139: pending_frame_host->loading_progress()) / 2; On 2015/02/24 13:54:55, clamy wrote: > Maybe use the max of the two rather than the average? I think we might be looking at this wrong. Do we ever get progress updates from a pending RFH? They're not based on the progress of the network, but rather on the load of the page (and its subframes) after it has committed. This means we'll never need to worry about the pending RFH's progress (unlike IsLoading, which becomes true before commit).
New patch, I have added a new unit test that covers all the cases where I found our test coverage to be lacking while debugging this patch. Note that this test currently does not pass. It also catches the mistake I made in an earlier patch set, that was left uncaught by the current tests. I am not exactly sure the way it is written is correct, though. On 2015/02/25 00:07:21, Charlie Reis wrote: > On 2015/02/24 13:54:55, clamy wrote: > > Besides this, I am quite uncertain of what is happening/needs to happen in > > WebContentsImpl::DidStartLoading with the reset of the load progress. I think > we > > should make clear what we want to achieve here. > > > > From what I understand, currently a new main frame load will trigger a reset > of > > the load status of all subframes. In particular if a new cross-site navigation > > gets canceled before it commits, then it would seem that no node in the > > FrameTree is loading, even though the subframes have not been destroyed and > may > > still be loading. At the same time, it makes more sense from a user point of > > view to consider the load status of the latest navigation (and we don't want > to > > mix the status of the new main frame load and the current subframe loads). > > Therefore, I wonder if instead of resetting the subframe status upon the start > > of navigation we should not be ignoring them in our computation of progress > > status while there is an ongoing main frame navigation that has not committed > > yet. > > You're referring to this code, right? > https://codereview.chromium.org/819673002 > > I'm definitely open to handling that in a more principled way. I am not sure how to "properly" track this without resorting to some sort of tracking in WebContentsImpl, which would bring us back to the original problem we are trying to solve here. https://codereview.chromium.org/925623002/diff/120001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/925623002/diff/120001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:139: pending_frame_host->loading_progress()) / 2; On 2015/02/25 00:07:21, Charlie Reis wrote: > On 2015/02/24 13:54:55, clamy wrote: > > Maybe use the max of the two rather than the average? > > I think we might be looking at this wrong. Do we ever get progress updates from > a pending RFH? They're not based on the progress of the network, but rather on > the load of the page (and its subframes) after it has committed. This means > we'll never need to worry about the pending RFH's progress (unlike IsLoading, > which becomes true before commit). This seems fairly reasonable. I am changing it to track the current RFH. https://codereview.chromium.org/925623002/diff/120001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/925623002/diff/120001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:216: bool is_loading() const { return is_loading_; } On 2015/02/24 13:54:55, clamy wrote: > nit: add an empty line above (same below). I am not exactly sure what the rule is here, I cannot find anything related to that case in the Google C++ style guide. We have some method definitions just above these ones that do not have space between them either. In the current frame_tree_node.h, these methods do not have an empty line between them, is that inconsistent with the style guide and/or the rest of the code base? https://codereview.chromium.org/925623002/diff/120001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:683: // Boolean value indicating whether this RFH in the process of loading a On 2015/02/24 13:54:55, clamy wrote: > "is in the process..." Done.
Thanks! A few more comments. https://codereview.chromium.org/925623002/diff/120001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/925623002/diff/120001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:216: bool is_loading() const { return is_loading_; } On 2015/02/26 13:44:12, Fabrice wrote: > On 2015/02/24 13:54:55, clamy wrote: > > nit: add an empty line above (same below). > > I am not exactly sure what the rule is here, I cannot find anything related to > that case in the Google C++ style guide. We have some method definitions just > above these ones that do not have space between them either. > In the current frame_tree_node.h, these methods do not have an empty line > between them, is that inconsistent with the style guide and/or the rest of the > code base? Acknowledged. I guess it is fine in that case since they are the setter/getter of the same variable. Unfortunately we do not have precise guidelines on how to skip lines. Generally though we would add an extra line after an inlined method spanning several lines. https://codereview.chromium.org/925623002/diff/140001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (left): https://codereview.chromium.org/925623002/diff/140001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:3413: loading_progresses_.clear(); As mentionned offline, we want to keep the current behavior intact. Therefore I think you should replace this line with resetting the load progress of the current RFH in each FTN to kLoadingProgressNotStarted. Then you would need not to account for frames with a load progress of not started in the count. Other option, you add a boolean to the RFH/FTN indicating whether it should be taken into account in the total load computation. It is reset to false for all frames there, and set to true when a frame starts loading. https://codereview.chromium.org/925623002/diff/140001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/925623002/diff/140001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2887: // one pending navigation. I think currently the test is not doing quite what you intend it to do. Something better would be along the lines of: - Navigate and commit url1. - Start cross-site navigation to url2. This creates a pending RFH. - Receive a DidStartLoading IPC from the pending RFH. - Receive a DidStartLoading IPC from the current RFH (reflecting a renderer-initiated navigation). - Get a DidNavigate IPC from the current RFH for a non-user initiated navigation. You should still have the pending RFH at that point. - Get the DidStopLoading from the current RFH. You should still be loading. - At that point the pending RFH commits ie you receive a DidNavigate from it. The original RFH is swapped out. - Finally you get the DidStopLoading from the (now) current RFH. You should have stopped loading at that point. https://codereview.chromium.org/925623002/diff/140001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2924: // still be loading. At that point the original RFH is swapped out (it got swapped out following TestDidNavigate). Therefore the DidStopLoading IPC will be filtered out, and WebContents should not even be informed.
Few more comments. https://codereview.chromium.org/925623002/diff/160001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/925623002/diff/160001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.h:113: // Returns true if either the current or pending RenderFrameHost from the nit: Let's avoid putting implementation details in the comment. It somehow figures out whether the current node is in a loading state : ). https://codereview.chromium.org/925623002/diff/160001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/925623002/diff/160001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:99: // minimum progress value. If we plan to hide these values internally in the future, which I think we should, it is good to put a TODO mentioning it. https://codereview.chromium.org/925623002/diff/160001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:684: // Boolean value indicating whether this RFH is in the process of loading a Let's drop the "Boolean" part of the comment, it is obvious what the type is from the next line. Same for the next one too. https://codereview.chromium.org/925623002/diff/160001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/925623002/diff/160001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:2902: // Then clean-up the states. nit: Empty line before the comment or combine with the previous comment. https://codereview.chromium.org/925623002/diff/160001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/925623002/diff/160001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2895: // Navigate the RenderFrame, simulate the DidStartLoading, and commit. nit: RenderFrameHost https://codereview.chromium.org/925623002/diff/160001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2912: orig_rfh->PrepareForCommit(new_url); Should this be in an if statement? I thought PrepareForCommit does the right thing in all cases.
Thanks again for the comments. I changed the loading tracking per clamy's suggestion and (I think) fixed the new test to do the right thing. https://codereview.chromium.org/925623002/diff/140001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (left): https://codereview.chromium.org/925623002/diff/140001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:3413: loading_progresses_.clear(); On 2015/02/26 16:05:00, clamy wrote: > As mentionned offline, we want to keep the current behavior intact. Therefore I > think you should replace this line with resetting the load progress of the > current RFH in each FTN to kLoadingProgressNotStarted. Then you would need not > to account for frames with a load progress of not started in the count. > > Other option, you add a boolean to the RFH/FTN indicating whether it should be > taken into account in the total load computation. It is reset to false for all > frames there, and set to true when a frame starts loading. I like the first option because it fixes a whole bunch of problems, including some we have with the current implementation. I'm going to do that, thanks a lot! https://codereview.chromium.org/925623002/diff/140001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/925623002/diff/140001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2887: // one pending navigation. On 2015/02/26 16:05:00, clamy wrote: > I think currently the test is not doing quite what you intend it to do. > > Something better would be along the lines of: > - Navigate and commit url1. > - Start cross-site navigation to url2. This creates a pending RFH. > - Receive a DidStartLoading IPC from the pending RFH. > - Receive a DidStartLoading IPC from the current RFH (reflecting a > renderer-initiated navigation). > - Get a DidNavigate IPC from the current RFH for a non-user initiated > navigation. You should still have the pending RFH at that point. > - Get the DidStopLoading from the current RFH. You should still be loading. > - At that point the pending RFH commits ie you receive a DidNavigate from it. > The original RFH is swapped out. > - Finally you get the DidStopLoading from the (now) current RFH. You should have > stopped loading at that point. Thanks a lot for the explanation! I think I'm now doing the right thing. https://codereview.chromium.org/925623002/diff/140001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2924: // still be loading. On 2015/02/26 16:05:00, clamy wrote: > At that point the original RFH is swapped out (it got swapped out following > TestDidNavigate). Therefore the DidStopLoading IPC will be filtered out, and > WebContents should not even be informed. Acknowledged. https://codereview.chromium.org/925623002/diff/160001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.h (right): https://codereview.chromium.org/925623002/diff/160001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.h:113: // Returns true if either the current or pending RenderFrameHost from the On 2015/02/26 23:30:33, nasko wrote: > nit: Let's avoid putting implementation details in the comment. It somehow > figures out whether the current node is in a loading state : ). Acknowledged. https://codereview.chromium.org/925623002/diff/160001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/925623002/diff/160001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:99: // minimum progress value. On 2015/02/26 23:30:33, nasko wrote: > If we plan to hide these values internally in the future, which I think we > should, it is good to put a TODO mentioning it. Acknowledged. https://codereview.chromium.org/925623002/diff/160001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:684: // Boolean value indicating whether this RFH is in the process of loading a On 2015/02/26 23:30:33, nasko wrote: > Let's drop the "Boolean" part of the comment, it is obvious what the type is > from the next line. Same for the next one too. Acknowledged. https://codereview.chromium.org/925623002/diff/160001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/925623002/diff/160001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:2902: // Then clean-up the states. On 2015/02/26 23:30:33, nasko wrote: > nit: Empty line before the comment or combine with the previous comment. Acknowledged. https://codereview.chromium.org/925623002/diff/160001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/925623002/diff/160001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2895: // Navigate the RenderFrame, simulate the DidStartLoading, and commit. On 2015/02/26 23:30:34, nasko wrote: > nit: RenderFrameHost Acknowledged. https://codereview.chromium.org/925623002/diff/160001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2912: orig_rfh->PrepareForCommit(new_url); On 2015/02/26 23:30:33, nasko wrote: > Should this be in an if statement? I thought PrepareForCommit does the right > thing in all cases. You are correct, fixed.
Thanks! A few more comments. https://codereview.chromium.org/925623002/diff/180001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/925623002/diff/180001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:124: if (pending_frame_host != nullptr) Could you add a TODO to have a PlzNavigate specific implementation as this will not work. https://codereview.chromium.org/925623002/diff/180001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/925623002/diff/180001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:163: bool ForEachCollectLoadProgress(double* progress, nit: I think I prefered the version without the ForEach prefix. https://codereview.chromium.org/925623002/diff/180001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/925623002/diff/180001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2892: // WebContentsImpl should have stopped loading at this point. I think this is a bit too long as a comment. Instead of the complete description, which people can find by reading the comments inside the test, how about the following: "This can happen if a same-process non user-initiated navigation completes while there is an ongoing cross-process navigation."? https://codereview.chromium.org/925623002/diff/180001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2894: const GURL url1("http://www.chromium.org"); Since these are const values, rename them kUrl1/kUrl2. https://codereview.chromium.org/925623002/diff/180001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2899: // Navigate the RenderFrameHost and commit. The frame should still be loading. You can replace all this below with contents()->NavigateAndCommit(kUrl1). https://codereview.chromium.org/925623002/diff/180001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2907: // Navigate to new site and commit. The frame should still be loading. The comment should indicate that this is a browser-initiated cross-process navigation to url2. Also you are not committing the pending navigation at that point (otherwise we don't test what we want since the pending_rfh will be swapped in). https://codereview.chromium.org/925623002/diff/180001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2910: current_rfh->PrepareForCommit(url2); I don't think we should get to that step right now. https://codereview.chromium.org/925623002/diff/180001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2918: // loading. What you want to do here is simulate a renderer-initiated non user-initiated navigation. The comment should reflect that. In order to have the test properly work with PlzNavigate you may want to have a look a carlosk's patch https://codereview.chromium.org/953503002/ which introduces a TestRenderFrameHost::SendRendererInitiatedNavigationRequest that you will need to use here when the patch lands. https://codereview.chromium.org/925623002/diff/180001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2927: contents()->TestDidNavigate(pending_rfh, 1, url2, The actual commit in the pending RFH should happen after the DidStopLoading in the current one (otherwise as in the previous version of the patch you simulate an IPC on a swapped out RFH that will get filtered out). https://codereview.chromium.org/925623002/diff/180001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2932: // loading and the pending RFH should now be the current RFH. You should simulate the commit of the current RFH provisional load before the DidStopLoading and make sure we still have a pending rfh at that point (see RenderFrameHostManagerTest.PageDoesBackAndReload for when that happens). https://codereview.chromium.org/925623002/diff/180001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2935: EXPECT_TRUE(contents()->IsLoading()); Following this you should commit the navigation in the pending RFH, and test what you have below (it becomes true following the call to contents()->TestDidNavigate(pending_rfh, 1, url2, ui::PAGE_TRANSITION_TYPED);).
I have only a few minor comments for now. https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... File content/browser/web_contents/web_contents_impl.h (right): https://codereview.chromium.org/925623002/diff/1/content/browser/web_contents... content/browser/web_contents/web_contents_impl.h:970: // false otherwise. On 2015/02/24 13:14:46, Fabrice wrote: > On 2015/02/13 15:05:41, carlosk wrote: > > nit: the otherwise case here seems redundant. > > Not sure about the style, but there are some others like it in the codebase: > https://code.google.com/p/chromium/codesearch#search/&q=%22false%20otherwise%... Acknowledged. https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... File content/browser/web_contents/web_contents_impl.cc (left): https://codereview.chromium.org/925623002/diff/60001/content/browser/web_cont... content/browser/web_contents/web_contents_impl.cc:3401: return; On 2015/02/24 13:14:47, Fabrice wrote: > On 2015/02/23 10:41:15, carlosk wrote: > > You *might* be able replace this if-check with |(progress == 0)| as this > *seems* > > to reflect what this early return used to do within the new implementation > > context (meaning no nodes are loading). > > The loading progress tracking that is done here is inaccurate at best, it is an > estimate. This new way is no better nor worse. I do not think this test is > necessary anyway because the test on line 3405 will return early if |(progress > == 0.0)| anyway. Acknowledged. https://codereview.chromium.org/925623002/diff/180001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/925623002/diff/180001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:686: // Indicates whether this RFH is in the process of loading a document or not. nit: s/RFH/RenderFrameHost Same below. https://codereview.chromium.org/925623002/diff/180001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/925623002/diff/180001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2945: } A couple general comments: * Replace RFH in your comments with RenderFrameHost * I agree we probably want to simulate a few navigation commit steps to make sure the loading state is working properly at that point.
Thanks a lot clamy for the offline help with the test! I think everything should be doing the right things now. https://codereview.chromium.org/925623002/diff/180001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/925623002/diff/180001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:124: if (pending_frame_host != nullptr) On 2015/03/02 10:30:58, clamy wrote: > Could you add a TODO to have a PlzNavigate specific implementation as this will > not work. Done. https://codereview.chromium.org/925623002/diff/180001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/925623002/diff/180001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:686: // Indicates whether this RFH is in the process of loading a document or not. On 2015/03/02 14:17:29, carlosk wrote: > nit: s/RFH/RenderFrameHost > > Same below. In this file, "RFH" is sometimes used in other places to refer specifically to "this RFH" instance. Though the use seems inconsistent so I'm changing it to the full name. Done. https://codereview.chromium.org/925623002/diff/180001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/925623002/diff/180001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:163: bool ForEachCollectLoadProgress(double* progress, On 2015/03/02 10:30:58, clamy wrote: > nit: I think I prefered the version without the ForEach prefix. I noticed the other helper functions below use a ForEach prefix but the ones above do not. One of them is probably wrong, I prefer the ForEach prefix but am open to suggestions to change it back. https://codereview.chromium.org/925623002/diff/180001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/925623002/diff/180001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2892: // WebContentsImpl should have stopped loading at this point. On 2015/03/02 10:30:59, clamy wrote: > I think this is a bit too long as a comment. Instead of the complete > description, which people can find by reading the comments inside the test, how > about the following: "This can happen if a same-process non user-initiated > navigation completes while there is an ongoing cross-process navigation."? Done. https://codereview.chromium.org/925623002/diff/180001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2894: const GURL url1("http://www.chromium.org"); On 2015/03/02 10:30:59, clamy wrote: > Since these are const values, rename them kUrl1/kUrl2. Done. https://codereview.chromium.org/925623002/diff/180001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2899: // Navigate the RenderFrameHost and commit. The frame should still be loading. On 2015/03/02 10:30:59, clamy wrote: > You can replace all this below with contents()->NavigateAndCommit(kUrl1). Done. https://codereview.chromium.org/925623002/diff/180001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2907: // Navigate to new site and commit. The frame should still be loading. On 2015/03/02 10:30:59, clamy wrote: > The comment should indicate that this is a browser-initiated cross-process > navigation to url2. Also you are not committing the pending navigation at that > point (otherwise we don't test what we want since the pending_rfh will be > swapped in). Done. https://codereview.chromium.org/925623002/diff/180001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2910: current_rfh->PrepareForCommit(url2); On 2015/03/02 10:30:58, clamy wrote: > I don't think we should get to that step right now. Done. https://codereview.chromium.org/925623002/diff/180001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2918: // loading. On 2015/03/02 10:30:59, clamy wrote: > What you want to do here is simulate a renderer-initiated non user-initiated > navigation. The comment should reflect that. In order to have the test properly > work with PlzNavigate you may want to have a look a carlosk's patch > https://codereview.chromium.org/953503002/ which introduces a > TestRenderFrameHost::SendRendererInitiatedNavigationRequest that you will need > to use here when the patch lands. I suggest proceeding in another fashion, this test is going to fail with PlzNavigate anyway because we don't properly call Did(Start|Stop)Loading. I will add a TODO to revisit this test when I add the proper calls for PlzNavigate. Does that sound good to you? https://codereview.chromium.org/925623002/diff/180001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2927: contents()->TestDidNavigate(pending_rfh, 1, url2, On 2015/03/02 10:30:59, clamy wrote: > The actual commit in the pending RFH should happen after the DidStopLoading in > the current one (otherwise as in the previous version of the patch you simulate > an IPC on a swapped out RFH that will get filtered out). Done. https://codereview.chromium.org/925623002/diff/180001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2932: // loading and the pending RFH should now be the current RFH. On 2015/03/02 10:30:59, clamy wrote: > You should simulate the commit of the current RFH provisional load before the > DidStopLoading and make sure we still have a pending rfh at that point (see > RenderFrameHostManagerTest.PageDoesBackAndReload for when that happens). Done. https://codereview.chromium.org/925623002/diff/180001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2935: EXPECT_TRUE(contents()->IsLoading()); On 2015/03/02 10:30:59, clamy wrote: > Following this you should commit the navigation in the pending RFH, and test > what you have below (it becomes true following the call to > contents()->TestDidNavigate(pending_rfh, 1, url2, ui::PAGE_TRANSITION_TYPED);). Done. https://codereview.chromium.org/925623002/diff/180001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2945: } On 2015/03/02 14:17:29, carlosk wrote: > A couple general comments: > * Replace RFH in your comments with RenderFrameHost > * I agree we probably want to simulate a few navigation commit steps to make > sure the loading state is working properly at that point. Done.
The CL is looking good, but please run some tryjobs so we can have an idea if something obvious is breaking.
This is much improved since I last looked-- thanks for the effort. Mainly style nits below, and +1 to try jobs. https://codereview.chromium.org/925623002/diff/200001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/925623002/diff/200001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:123: DCHECK(current_frame_host != nullptr); nit: Drop the "!= nullptr" The check is equivalent and easier to read. https://codereview.chromium.org/925623002/diff/200001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:128: else You don't need this else, since the previous branch returns. Actually, we can clean this up a bit further: if (pending_frame_host && pending_frame_host->is_loading()) return true; return current_frame_host->is_loading(); https://codereview.chromium.org/925623002/diff/200001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:136: DCHECK(current_frame_host != nullptr); nit: Drop the "!= nullptr" https://codereview.chromium.org/925623002/diff/200001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/925623002/diff/200001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:215: void set_is_loading(bool is_loading) { Please document these, since it's unclear whether other callers in content should use these or the ones in FrameTreeNode. We should make it clear that FrameTreeNode::IsLoading is the place to ask, and that these are just inputs to it. https://codereview.chromium.org/925623002/diff/200001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/925623002/diff/200001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:163: bool ForEachCollectLoadProgress(double* progress, Please don't prefix these with ForEach. The ForEachFrameInternal and ForEachPendingFrameInternal methods below are not comparable; they're helper methods for actually calling the callbacks on each node. These new methods are closer to CollectSites. https://codereview.chromium.org/925623002/diff/200001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/925623002/diff/200001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2898: // be a pending RenderFrameHost and the frame should be loading. The comment says "the frame should be loading" but the code is checking whether the WebContents is loading (same below). Can we check the frame as well?
Thanks for the comments, try jobs are running. https://codereview.chromium.org/925623002/diff/200001/content/browser/frame_h... File content/browser/frame_host/frame_tree_node.cc (right): https://codereview.chromium.org/925623002/diff/200001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:123: DCHECK(current_frame_host != nullptr); On 2015/03/03 06:32:22, Charlie Reis wrote: > nit: Drop the "!= nullptr" > The check is equivalent and easier to read. Done. https://codereview.chromium.org/925623002/diff/200001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:128: else On 2015/03/03 06:32:22, Charlie Reis wrote: > You don't need this else, since the previous branch returns. > > Actually, we can clean this up a bit further: > > if (pending_frame_host && pending_frame_host->is_loading()) > return true; > > return current_frame_host->is_loading(); Done. I like leaving the else because it makes for easier readability imo, but your way looks better. https://codereview.chromium.org/925623002/diff/200001/content/browser/frame_h... content/browser/frame_host/frame_tree_node.cc:136: DCHECK(current_frame_host != nullptr); On 2015/03/03 06:32:22, Charlie Reis wrote: > nit: Drop the "!= nullptr" Done. https://codereview.chromium.org/925623002/diff/200001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/925623002/diff/200001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:215: void set_is_loading(bool is_loading) { On 2015/03/03 06:32:22, Charlie Reis wrote: > Please document these, since it's unclear whether other callers in content > should use these or the ones in FrameTreeNode. We should make it clear that > FrameTreeNode::IsLoading is the place to ask, and that these are just inputs to > it. Done. I clarified the loading progress too. https://codereview.chromium.org/925623002/diff/200001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/925623002/diff/200001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:163: bool ForEachCollectLoadProgress(double* progress, On 2015/03/03 06:32:23, Charlie Reis wrote: > Please don't prefix these with ForEach. The ForEachFrameInternal and > ForEachPendingFrameInternal methods below are not comparable; they're helper > methods for actually calling the callbacks on each node. These new methods are > closer to CollectSites. Done. https://codereview.chromium.org/925623002/diff/200001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/925623002/diff/200001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2898: // be a pending RenderFrameHost and the frame should be loading. On 2015/03/03 06:32:23, Charlie Reis wrote: > The comment says "the frame should be loading" but the code is checking whether > the WebContents is loading (same below). Can we check the frame as well? We should not check the frame before the call to DidStartLoading because it won't have started loading before then. I am changing the comment to state that the WebContents should be loading instead. I think checking the individual RFH status would be testing for implementation rather than the flow in WebContents.
Almost there! https://codereview.chromium.org/925623002/diff/220001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/925623002/diff/220001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2902: ASSERT_NE(pending_rfh, nullptr); In the lines of what was mentioned earlier this should be change this to ASSERT_FALSE(pending_rfh). https://codereview.chromium.org/925623002/diff/220001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2906: // navigation and sends a DidStartLoading IPC. The WebContents still be nit: ... WebContents *should* still... https://codereview.chromium.org/925623002/diff/220001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2922: current_rfh->SendNavigate(1, kUrl3); I think the comments are a bit out of place. This is the point where the renderer-initiated non user-initiated navigation is received. https://codereview.chromium.org/925623002/diff/220001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2932: EXPECT_EQ(contents()->GetPendingMainFrame(), nullptr); Replace with: EXPECT_FALSE(contents()->GetPendingMainFrame()); But I do see that in this file there's a lot of instances of pointer checks using |== NULL| and |!= NULL|.
Quick update on the try jobs, android_rel_tests_recipe is currently failing due to https://code.google.com/p/chromium/issues/detail?id=463436
Thanks! A few more comments, besides teh ones from carlosk@. https://codereview.chromium.org/925623002/diff/220001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/925623002/diff/220001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:3448: bool WebContentsImpl::IsFrameTreeLoading() { Suggestion: replace this private member function with a helper function in an anonymous namespace taking the FrameTree as an argument. https://codereview.chromium.org/925623002/diff/220001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/925623002/diff/220001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2922: current_rfh->SendNavigate(1, kUrl3); On 2015/03/03 15:29:12, carlosk wrote: > I think the comments are a bit out of place. This is the point where the > renderer-initiated non user-initiated navigation is received. On the renderer side it started when we received the DidStartLoading IPC. Maybe something like "Simulate the commit and DidStopLoading from the renderer-initiated in the current RenderFrameHost" is a bit clearer.
https://codereview.chromium.org/925623002/diff/220001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/925623002/diff/220001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:3448: bool WebContentsImpl::IsFrameTreeLoading() { On 2015/03/03 17:01:09, clamy wrote: > Suggestion: replace this private member function with a helper function in an > anonymous namespace taking the FrameTree as an argument. Done. https://codereview.chromium.org/925623002/diff/220001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/925623002/diff/220001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2902: ASSERT_NE(pending_rfh, nullptr); On 2015/03/03 15:29:12, carlosk wrote: > In the lines of what was mentioned earlier this should be change this to > ASSERT_FALSE(pending_rfh). In this specific case I prefer it for readability and semantic purposes. You're not testing a boolean but a pointer. https://codereview.chromium.org/925623002/diff/220001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2906: // navigation and sends a DidStartLoading IPC. The WebContents still be On 2015/03/03 15:29:13, carlosk wrote: > nit: ... WebContents *should* still... I accidentally the verb. Done. https://codereview.chromium.org/925623002/diff/220001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2922: current_rfh->SendNavigate(1, kUrl3); On 2015/03/03 17:01:09, clamy wrote: > On 2015/03/03 15:29:12, carlosk wrote: > > I think the comments are a bit out of place. This is the point where the > > renderer-initiated non user-initiated navigation is received. > > On the renderer side it started when we received the DidStartLoading IPC. Maybe > something like "Simulate the commit and DidStopLoading from the > renderer-initiated in the current RenderFrameHost" is a bit clearer. Done. https://codereview.chromium.org/925623002/diff/220001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2932: EXPECT_EQ(contents()->GetPendingMainFrame(), nullptr); On 2015/03/03 15:29:13, carlosk wrote: > Replace with: EXPECT_FALSE(contents()->GetPendingMainFrame()); > > But I do see that in this file there's a lot of instances of pointer checks > using |== NULL| and |!= NULL|. Same as above.
Thanks! Lgtm, provided the ASSERT_NE in the tests are changed to ASSERT_FALSE. https://codereview.chromium.org/925623002/diff/220001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/925623002/diff/220001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2902: ASSERT_NE(pending_rfh, nullptr); On 2015/03/04 18:17:58, Fabrice wrote: > On 2015/03/03 15:29:12, carlosk wrote: > > In the lines of what was mentioned earlier this should be change this to > > ASSERT_FALSE(pending_rfh). > > In this specific case I prefer it for readability and semantic purposes. You're > not testing a boolean but a pointer. We use ASSERT_FALSE(ptr) everywhere in the unit tests and ASSERT_NE(ptr, nullptr) nowhere. As a rule of thumb, you should try to align yourself to the style of what you are modifying. https://codereview.chromium.org/925623002/diff/220001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2932: EXPECT_EQ(contents()->GetPendingMainFrame(), nullptr); On 2015/03/04 18:17:58, Fabrice wrote: > On 2015/03/03 15:29:13, carlosk wrote: > > Replace with: EXPECT_FALSE(contents()->GetPendingMainFrame()); > > > > But I do see that in this file there's a lot of instances of pointer checks > > using |== NULL| and |!= NULL|. > > Same as above. Same here, please do the change asked by carlosk@
Agreed on the EXPECT_FALSE(ptr) convention, which is widespread in content/. Otherwise LGTM with nits once the compile error is fixed. https://codereview.chromium.org/925623002/diff/240001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/925623002/diff/240001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:220: // Returns this RenderFrameHost loading state. This method is only to be used nit: RenderFrameHost's nit: used by (Same in loading_progress below) https://codereview.chromium.org/925623002/diff/240001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:225: // Sets this RenderFrameHost loading progress (from 0 to 1). nit: RenderFrameHost's https://codereview.chromium.org/925623002/diff/240001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:700: // Used to represent this RenderFrameHost loading progress (from 0 to 1). nit: RenderFrameHost's https://codereview.chromium.org/925623002/diff/240001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/925623002/diff/240001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:165: int* frame_count, nit: Wrong indent. https://codereview.chromium.org/925623002/diff/240001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/925623002/diff/240001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2909: FrameHostMsg_DidStartLoading(current_rfh->GetRoutingID(), false)); nit: Wrong indent. https://codereview.chromium.org/925623002/diff/240001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2914: pending_rfh->PrepareForCommit(kUrl2); Compile error here? https://codereview.chromium.org/925623002/diff/240001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2929: // Commit the navigation, the formerly pending RenderFrameHost should now be Run-on sentence. Use either a period or a semicolon. https://codereview.chromium.org/925623002/diff/240001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2935: ASSERT_EQ(new_current_rfh, pending_rfh); EXPECT_EQ (Save ASSERT_* for things that prevent you from getting any further in the test, since it's useful to produce as much output as we can with EXPECT_*.)
Thanks folks, seems good now! clamy@, carlosk@, could you do a final sanity check before I send it through CQ? https://codereview.chromium.org/925623002/diff/220001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/925623002/diff/220001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2902: ASSERT_NE(pending_rfh, nullptr); On 2015/03/04 18:28:49, clamy wrote: > On 2015/03/04 18:17:58, Fabrice wrote: > > On 2015/03/03 15:29:12, carlosk wrote: > > > In the lines of what was mentioned earlier this should be change this to > > > ASSERT_FALSE(pending_rfh). > > > > In this specific case I prefer it for readability and semantic purposes. > You're > > not testing a boolean but a pointer. > > We use ASSERT_FALSE(ptr) everywhere in the unit tests and ASSERT_NE(ptr, > nullptr) nowhere. As a rule of thumb, you should try to align yourself to the > style of what you are modifying. We actually want ASSERT_TRUE here. Done. https://codereview.chromium.org/925623002/diff/220001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2932: EXPECT_EQ(contents()->GetPendingMainFrame(), nullptr); On 2015/03/04 18:28:49, clamy wrote: > On 2015/03/04 18:17:58, Fabrice wrote: > > On 2015/03/03 15:29:13, carlosk wrote: > > > Replace with: EXPECT_FALSE(contents()->GetPendingMainFrame()); > > > > > > But I do see that in this file there's a lot of instances of pointer checks > > > using |== NULL| and |!= NULL|. > > > > Same as above. > > Same here, please do the change asked by carlosk@ Done. https://codereview.chromium.org/925623002/diff/240001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/925623002/diff/240001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:220: // Returns this RenderFrameHost loading state. This method is only to be used On 2015/03/05 04:46:18, Charlie Reis wrote: > nit: RenderFrameHost's > nit: used by > > (Same in loading_progress below) Done. https://codereview.chromium.org/925623002/diff/240001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:225: // Sets this RenderFrameHost loading progress (from 0 to 1). On 2015/03/05 04:46:18, Charlie Reis wrote: > nit: RenderFrameHost's Done. https://codereview.chromium.org/925623002/diff/240001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:700: // Used to represent this RenderFrameHost loading progress (from 0 to 1). On 2015/03/05 04:46:18, Charlie Reis wrote: > nit: RenderFrameHost's Done. https://codereview.chromium.org/925623002/diff/240001/content/browser/web_con... File content/browser/web_contents/web_contents_impl.cc (right): https://codereview.chromium.org/925623002/diff/240001/content/browser/web_con... content/browser/web_contents/web_contents_impl.cc:165: int* frame_count, On 2015/03/05 04:46:18, Charlie Reis wrote: > nit: Wrong indent. Done. https://codereview.chromium.org/925623002/diff/240001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/925623002/diff/240001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2909: FrameHostMsg_DidStartLoading(current_rfh->GetRoutingID(), false)); On 2015/03/05 04:46:18, Charlie Reis wrote: > nit: Wrong indent. Done. https://codereview.chromium.org/925623002/diff/240001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2914: pending_rfh->PrepareForCommit(kUrl2); On 2015/03/05 04:46:18, Charlie Reis wrote: > Compile error here? I needed to rebase locally. Done. https://codereview.chromium.org/925623002/diff/240001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2929: // Commit the navigation, the formerly pending RenderFrameHost should now be On 2015/03/05 04:46:18, Charlie Reis wrote: > Run-on sentence. Use either a period or a semicolon. Done. https://codereview.chromium.org/925623002/diff/240001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2935: ASSERT_EQ(new_current_rfh, pending_rfh); On 2015/03/05 04:46:19, Charlie Reis wrote: > EXPECT_EQ > > (Save ASSERT_* for things that prevent you from getting any further in the test, > since it's useful to produce as much output as we can with EXPECT_*.) Done.
LGTM
LGTM with a couple of nits! https://codereview.chromium.org/925623002/diff/300001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/925623002/diff/300001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:101: // IPCs are moved from WebContentsImpl to RFHI. nit: s/RFHI/RenderFrameHost/ https://codereview.chromium.org/925623002/diff/300001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:700: // Used to represent this RenderFrameHost's loading progress (from 0 to 1). nit: s/represent/track/ https://codereview.chromium.org/925623002/diff/300001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/925623002/diff/300001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2921: FrameHostMsg_DidStartLoading(pending_rfh->GetRoutingID(), false)); For browser initiated navigations, we mark the frame as started loading before we hear from the renderer. Just noting it here, as when more code moves around, the DCHECK you have in WebContentsImpl::OnDidStartLoading will hit.
Thanks folks! https://codereview.chromium.org/925623002/diff/300001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.h (right): https://codereview.chromium.org/925623002/diff/300001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:101: // IPCs are moved from WebContentsImpl to RFHI. On 2015/03/05 14:29:23, nasko wrote: > nit: s/RFHI/RenderFrameHost/ Done. https://codereview.chromium.org/925623002/diff/300001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.h:700: // Used to represent this RenderFrameHost's loading progress (from 0 to 1). On 2015/03/05 14:29:23, nasko wrote: > nit: s/represent/track/ Done. https://codereview.chromium.org/925623002/diff/300001/content/browser/web_con... File content/browser/web_contents/web_contents_impl_unittest.cc (right): https://codereview.chromium.org/925623002/diff/300001/content/browser/web_con... content/browser/web_contents/web_contents_impl_unittest.cc:2921: FrameHostMsg_DidStartLoading(pending_rfh->GetRoutingID(), false)); On 2015/03/05 14:29:23, nasko wrote: > For browser initiated navigations, we mark the frame as started loading before > we hear from the renderer. Just noting it here, as when more code moves around, > the DCHECK you have in WebContentsImpl::OnDidStartLoading will hit. Acknowledged. I'll keep that in mind when moving the IPCs.
The CQ bit was checked by fdegans@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org, creis@chromium.org, nasko@chromium.org, carlosk@chromium.org Link to the patchset: https://codereview.chromium.org/925623002/#ps320001 (title: "Nitses.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/925623002/320001
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/bda82dd8f2286787212cb92c694a999f08834c7b Cr-Commit-Position: refs/heads/master@{#319267}
Message was sent while issue was closed.
A revert of this CL (patchset #17 id:320001) has been created in https://codereview.chromium.org/983973002/ by tasak@google.com. The reason for reverting is: This patch causes the following layout tests' crashes: - fast/loader/stateobjects/replacestate-in-onunload.html: 2921 2922 // This method should never be called when the frame is loading. 2923 DCHECK(!rfh->is_loading()); 2924 #3 0x00007fffefece2d3 in content::WebContentsImpl::OnDidStartLoading ( this=0x181e00571b20, to_different_document=false) at ../../content/browser/web_contents/web_contents_impl.cc:2923 #4 0x00007fffefefa3e7 in DispatchToMethodImpl<content::WebContentsImpl, void (content::WebContentsImpl::*)(bool), bool, 0ul> (obj=0x181e00571b20, method= (void (content::WebContentsImpl::*)(content::WebContentsImpl * const, bool)) 0x7fffefece170 <content::WebContentsImpl::OnDidStartLoading(bool)>, arg=...) at ../../base/tuple.h:246 #5 0x00007fffefefa335 in DispatchToMethod<content::WebContentsImpl, void (content::WebContentsImpl::*)(bool), bool> (obj=0x181e00571b20, method= (void (content::WebContentsImpl::*)(content::WebContentsImpl * const, bool)) 0x7fffefece170 <content::WebContentsImpl::OnDidStartLoading(bool)>, arg=...) at ../../base/tuple.h:253 #6 0x00007fffefee4bf7 in FrameHostMsg_DidStartLoading::Dispatch<content::WebContentsImpl, content::WebContentsImpl, void, void (content::WebContentsImpl::*)(bool)> (msg=0x181e006fddd0, obj=0x181e00571b20, sender=0x181e00571b20, parameter=0x0, func= c.f. https://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.7%20%28d... . |
