Move DidStartLoading, DidStopLoading, DidChangeLoadProgress to RFHI.

Move DidStartLoading, DidStopLoading, DidChangeLoadProgress IPCs to RFHI.

This also adds a GetDelegate() method to the Navigator interface.

BUG=470082
Committed: https://crrev.com/a696e5116f6b81f317758ad609e110db5ed3c284
Cr-Commit-Position: refs/heads/master@{#325579}

[Fabrice (no longer in Chrome), 2015-04-14 16:54:11]

fdegans@chromium.org changed reviewers:
+ carlosk@chromium.org, clamy@chromium.org, creis@chromium.org,
nasko@chromium.org

[Fabrice (no longer in Chrome), 2015-04-14 16:54:12]

This is my attempt at implementing this patch differently. I have concerns about
this which I have written in comments.
Generally, I feel like this introduces more complexity in the codebase for very
little benefits. I also thought the idea behind FrameTree and FrameTreeNode was
to keep these simple and focused on holding the frames. From your comments on
the other patch, I get the feeling you want to move some of the logic from
WebContents to FrameTree, is this correct?

PS: I want to apologize in advance if any of my comments sound antagonistic,
that it is definitely not my intention.

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

https://codereview.chromium.org/1080143003/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree_node.cc:192:
frame_tree_->ResetLoadProgress();
This part was problematic.
We have "FrameTree:ResetLoadProgress" and
"WebContentsImpl::ResetLoadProgressState". Most of the times, we will want to
call the latter that will clean the states both on FrameTree and WebContents.
This call will only reset the load progress logic in FrameTree. This is not
actually a problem because we're calling UpdateLoadProgress below that will
actually take care of cleaning the states in WebContents but it is not obvious
when reading the code.

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

https://codereview.chromium.org/1080143003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:853:
frame_tree_node_->navigator()->DidStopLoading();
We have to go through the navigator here because calling
FrameTreeNode::DidStopLoading will not necessarily end up calling
WebContentsImpl::DidStopLoading.
That is also why we introduced the "page_start_loading_with_frame" argument on
the other CL.

https://codereview.chromium.org/1080143003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1426:
frame_tree_node_->DidStartLoading(to_different_document);
Here and below, it is not clear to me that this will end up calling into
WebContents and "doing the right thing". We're adding a new level of indirection
and increasing code complexity.
I had to reread what I was doing multiple times while checking tests were still
passing locally and found it confusing. I cannot imagine how it will be for
someone who has not written this part of the code.

https://codereview.chromium.org/1080143003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1442:
frame_tree_node_->DidStopLoading();
Same as above, it is not clear we'll end up in WebContents.

https://codereview.chromium.org/1080143003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1619:
frame_tree_node_->navigator()->DidStartLoading(frame_tree_node_, true);
Same as above, we need to go through the Navigator.

https://codereview.chromium.org/1080143003/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1080143003/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:3294:
delegate_->LoadProgressChanged(this, frame_tree_.GetLoadProgress());
This is the reason why we have to keep some of the load progress-related logic
in WebContents, to notify its delegate. This is not accessible from FrameTree.
The only way around it that I can think of would be to introduce a
FrameTreeDelegate implemented by WebContentsImpl.

https://codereview.chromium.org/1080143003/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl_browsertest.cc (right):

https://codereview.chromium.org/1080143003/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl_browsertest.cc:500: void
DidStartLoading() override {
This is one of the places where tests assume DidStartLoading/DidStopLoading are
only called once.

https://codereview.chromium.org/1080143003/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl_browsertest.cc:507: void
DidStopLoading() override {
Same as above.

[nasko, 2015-04-14 19:54:41]

This CL is a lot closer to what I had in mind. I think with a few small changes
it is going to be good to go.

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

https://codereview.chromium.org/1080143003/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree_node.cc:192:
frame_tree_->ResetLoadProgress();
On 2015/04/14 16:54:11, Fabrice wrote:
> This part was problematic.
> We have "FrameTree:ResetLoadProgress" and
> "WebContentsImpl::ResetLoadProgressState". Most of the times, we will want to
> call the latter that will clean the states both on FrameTree and WebContents.
> This call will only reset the load progress logic in FrameTree. This is not
> actually a problem because we're calling UpdateLoadProgress below that will
> actually take care of cleaning the states in WebContents but it is not obvious
> when reading the code.

If it isn't obvious, then it is a good candidate for an explicit comment
describing what is happening and how.

https://codereview.chromium.org/1080143003/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree_node.cc:194: // Notify the Navigator.
Navigator doesn't really need to know about this. The only reason I see for
these calls is to notify WebContents of the events through the delegate
interface. Let's think of how to structure this without going through Navigator.
For example, why not do this:

navigator()->delegate()->DidStartLoading(...)

Since this is exactly what the NavigatorImpl does anyway. I don't think this
level of indirection is needed here.

https://codereview.chromium.org/1080143003/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree_node.cc:204: 
nit: no need for empty line.

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

https://codereview.chromium.org/1080143003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:853:
frame_tree_node_->navigator()->DidStopLoading();
On 2015/04/14 16:54:11, Fabrice wrote:
> We have to go through the navigator here because calling
> FrameTreeNode::DidStopLoading will not necessarily end up calling
> WebContentsImpl::DidStopLoading.
> That is also why we introduced the "page_start_loading_with_frame" argument on
> the other CL.

I don't quite follow. During Navigate() we simulate DidStartLoading. If this is
FTN level operation, than it makes sense that undoing its work is calling
DidStopLoading on the FTN as well.
Why would we expect that WebContentsImpl::DidStopLoading is called for every
invocation of this code? Can't we have two sibling iframes start loading and
stop loading through those paths and we only need to do one call to
WebContentsImpl::DidStopLoading.

https://codereview.chromium.org/1080143003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1426:
frame_tree_node_->DidStartLoading(to_different_document);
On 2015/04/14 16:54:11, Fabrice wrote:
> Here and below, it is not clear to me that this will end up calling into
> WebContents and "doing the right thing". 

It is the job of the FTN to ensure that it does the right thing. The RFH only
needs to notify its FTN that it has entered loading state.

> We're adding a new level of indirection and increasing code complexity.

I actually find this a bit easier to reason about, as no RFH on its own knows
what "the right thing" to do is. It is only the FrameTree and its FTNs, having a
global picture of all loads that can determine it. So doing something in RFH
here is very counterintuitive to me.

> I had to reread what I was doing multiple times while checking tests were
still
> passing locally and found it confusing. I cannot imagine how it will be for
> someone who has not written this part of the code.

It will be useful to know what made it hard for you to understand what is going
on. Could you elaborate on that?
Code needs to be easy to reason about if we want to have a complex piece of
software, such as Chrome, to be maintainable.

https://codereview.chromium.org/1080143003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1442:
frame_tree_node_->DidStopLoading();
On 2015/04/14 16:54:11, Fabrice wrote:
> Same as above, it is not clear we'll end up in WebContents.

Why do we care to know here whether we end up in WebContents or not? RFH on its
own can never know that.

https://codereview.chromium.org/1080143003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1619:
frame_tree_node_->navigator()->DidStartLoading(frame_tree_node_, true);
On 2015/04/14 16:54:11, Fabrice wrote:
> Same as above, we need to go through the Navigator.

I don't think we need to. FTN is the object which has view of all RFHs
representing that frame, so it can work together with the FrameTree to figure
out what the global loading state is.

https://codereview.chromium.org/1080143003/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1080143003/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:3294:
delegate_->LoadProgressChanged(this, frame_tree_.GetLoadProgress());
On 2015/04/14 16:54:11, Fabrice wrote:
> This is the reason why we have to keep some of the load progress-related logic
> in WebContents, to notify its delegate. This is not accessible from FrameTree.
> The only way around it that I can think of would be to introduce a
> FrameTreeDelegate implemented by WebContentsImpl.

Yes, notifying the WebContents' delegate makes sense here. It also makes sense
for the FrameTree to tell WebContents when those events happen. What we don't
need here ideally is any state related to progress.

https://codereview.chromium.org/1080143003/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl_browsertest.cc (right):

https://codereview.chromium.org/1080143003/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl_browsertest.cc:500: void
DidStartLoading() override {
On 2015/04/14 16:54:12, Fabrice wrote:
> This is one of the places where tests assume DidStartLoading/DidStopLoading
are
> only called once.

Why would this be different after this change? This is an observer method, which
should only be called once when a frame in the tree starts loading.

[clamy, 2015-04-15 12:28:43]

Thanks! I find this version clearer, now that we know we can add more state to
the FrameTree/FrameTreeNode. One comment besides the ones from Nasko.

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

https://codereview.chromium.org/1080143003/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree.h:133: // Updates the overall load
progress and notifies the RenderFrameHostDelegate.
I think this the load progress update is in the NavigatorDelegate right?

[Fabrice (no longer in Chrome), 2015-04-15 15:15:03]

Thanks for the comments!
I made a new patch with updated comments. I changed the references to
"Navigator/NavigatorDelegate" to reference "WebContents" instead, this is
temporary until we decide how to end up notifying WebContents.
The Navigator interface has no "delegate()" method because only one of its two
implementations actually has a delegate. We would need to test for the presence
of a delegate if we were to add such a method. So I think the best way to do
that would be to add a FrameTreeDelegate class. WDYT?

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

https://codereview.chromium.org/1080143003/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree.h:133: // Updates the overall load
progress and notifies the RenderFrameHostDelegate.
On 2015/04/15 12:28:42, clamy wrote:
> I think this the load progress update is in the NavigatorDelegate right?

Yes, this was moved from the previous patch, fixed.

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

https://codereview.chromium.org/1080143003/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree_node.cc:192:
frame_tree_->ResetLoadProgress();
On 2015/04/14 19:54:41, nasko wrote:
> On 2015/04/14 16:54:11, Fabrice wrote:
> > This part was problematic.
> > We have "FrameTree:ResetLoadProgress" and
> > "WebContentsImpl::ResetLoadProgressState". Most of the times, we will want
to
> > call the latter that will clean the states both on FrameTree and
WebContents.
> > This call will only reset the load progress logic in FrameTree. This is not
> > actually a problem because we're calling UpdateLoadProgress below that will
> > actually take care of cleaning the states in WebContents but it is not
obvious
> > when reading the code.
> 
> If it isn't obvious, then it is a good candidate for an explicit comment
> describing what is happening and how.
I clarified the comments to ensure there is no confusion about the way things
are called and how WebContents ends up being notified.

https://codereview.chromium.org/1080143003/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree_node.cc:194: // Notify the Navigator.
On 2015/04/14 19:54:40, nasko wrote:
> Navigator doesn't really need to know about this. The only reason I see for
> these calls is to notify WebContents of the events through the delegate
> interface. Let's think of how to structure this without going through
Navigator.
> For example, why not do this:
> 
> navigator()->delegate()->DidStartLoading(...)
> 
> Since this is exactly what the NavigatorImpl does anyway. I don't think this
> level of indirection is needed here.

The issue is that Navigator does not have a delegate() method. NavigatorImpl
does, but not InterstitialPageNavigatorImpl.
What we really want is call into WebContents. I suggest adding a
FrameTreeDelegate class that will be implemented by WebContentsImpl. With the
new methods in FrameTreeNode, we no longer need the interface in Navigator.

https://codereview.chromium.org/1080143003/diff/1/content/browser/frame_host/...
content/browser/frame_host/frame_tree_node.cc:204: 
On 2015/04/14 19:54:41, nasko wrote:
> nit: no need for empty line.

Done.

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

https://codereview.chromium.org/1080143003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:853:
frame_tree_node_->navigator()->DidStopLoading();
On 2015/04/14 19:54:41, nasko wrote:
> On 2015/04/14 16:54:11, Fabrice wrote:
> > We have to go through the navigator here because calling
> > FrameTreeNode::DidStopLoading will not necessarily end up calling
> > WebContentsImpl::DidStopLoading.
> > That is also why we introduced the "page_start_loading_with_frame" argument
on
> > the other CL.
> 
> I don't quite follow. During Navigate() we simulate DidStartLoading. If this
is
> FTN level operation, than it makes sense that undoing its work is calling
> DidStopLoading on the FTN as well.
> Why would we expect that WebContentsImpl::DidStopLoading is called for every
> invocation of this code? Can't we have two sibling iframes start loading and
> stop loading through those paths and we only need to do one call to
> WebContentsImpl::DidStopLoading.

I think this is going to break some tests, trying it in next CL.

https://codereview.chromium.org/1080143003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1426:
frame_tree_node_->DidStartLoading(to_different_document);
On 2015/04/14 19:54:41, nasko wrote:
> On 2015/04/14 16:54:11, Fabrice wrote:
> > Here and below, it is not clear to me that this will end up calling into
> > WebContents and "doing the right thing". 
> 
> It is the job of the FTN to ensure that it does the right thing. The RFH only
> needs to notify its FTN that it has entered loading state.
> 
> > We're adding a new level of indirection and increasing code complexity.
> 
> I actually find this a bit easier to reason about, as no RFH on its own knows
> what "the right thing" to do is. It is only the FrameTree and its FTNs, having
a
> global picture of all loads that can determine it. So doing something in RFH
> here is very counterintuitive to me.
> 
> > I had to reread what I was doing multiple times while checking tests were
> still
> > passing locally and found it confusing. I cannot imagine how it will be for
> > someone who has not written this part of the code.
> 
> It will be useful to know what made it hard for you to understand what is
going
> on. Could you elaborate on that?
> Code needs to be easy to reason about if we want to have a complex piece of
> software, such as Chrome, to be maintainable.

The main issue is you now have to consider 4 different files when trying to
debug this piece of code. However, it is now made simpler because we consider
we're notifying the WebContents from FrameTree and FrameTreeNode while RFHI
simply notifies FrameTreeNode.
There are also two distinct "call stacks" that both end up in WebContents. One
is about the loading status and the other about the load progress. The first
goes "RFHI -> FTN -> WebContents" while the latter goes "RFHI -> FTN -> FT ->
WebContents". I have changed the comments to make that clearer.

https://codereview.chromium.org/1080143003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1442:
frame_tree_node_->DidStopLoading();
On 2015/04/14 19:54:41, nasko wrote:
> On 2015/04/14 16:54:11, Fabrice wrote:
> > Same as above, it is not clear we'll end up in WebContents.
> 
> Why do we care to know here whether we end up in WebContents or not? RFH on
its
> own can never know that.

Acknowledged.

https://codereview.chromium.org/1080143003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:1619:
frame_tree_node_->navigator()->DidStartLoading(frame_tree_node_, true);
On 2015/04/14 19:54:41, nasko wrote:
> On 2015/04/14 16:54:11, Fabrice wrote:
> > Same as above, we need to go through the Navigator.
> 
> I don't think we need to. FTN is the object which has view of all RFHs
> representing that frame, so it can work together with the FrameTree to figure
> out what the global loading state is.

Same as above, trying it in the next patch set.

https://codereview.chromium.org/1080143003/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1080143003/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl.cc:3294:
delegate_->LoadProgressChanged(this, frame_tree_.GetLoadProgress());
On 2015/04/14 19:54:41, nasko wrote:
> On 2015/04/14 16:54:11, Fabrice wrote:
> > This is the reason why we have to keep some of the load progress-related
logic
> > in WebContents, to notify its delegate. This is not accessible from
FrameTree.
> > The only way around it that I can think of would be to introduce a
> > FrameTreeDelegate implemented by WebContentsImpl.
> 
> Yes, notifying the WebContents' delegate makes sense here. It also makes sense
> for the FrameTree to tell WebContents when those events happen. What we don't
> need here ideally is any state related to progress.

Acknowledged.

https://codereview.chromium.org/1080143003/diff/1/content/browser/web_content...
File content/browser/web_contents/web_contents_impl_browsertest.cc (right):

https://codereview.chromium.org/1080143003/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl_browsertest.cc:500: void
DidStartLoading() override {
On 2015/04/14 19:54:41, nasko wrote:
> On 2015/04/14 16:54:12, Fabrice wrote:
> > This is one of the places where tests assume DidStartLoading/DidStopLoading
> are
> > only called once.
> 
> Why would this be different after this change? This is an observer method,
which
> should only be called once when a frame in the tree starts loading.

I agree this patch will not change things. This was intended to reply to a
comment in the other patch asking why we weren't doing more things in
Did[Start|Stop]Loading directly.

https://codereview.chromium.org/1080143003/diff/1/content/browser/web_content...
content/browser/web_contents/web_contents_impl_browsertest.cc:507: void
DidStopLoading() override {
On 2015/04/14 16:54:12, Fabrice wrote:
> Same as above.

Acknowledged.

[nasko, 2015-04-15 15:39:28]

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

https://codereview.chromium.org/1080143003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:853:
frame_tree_node_->navigator()->DidStopLoading();
On 2015/04/15 15:15:03, Fabrice wrote:
> On 2015/04/14 19:54:41, nasko wrote:
> > On 2015/04/14 16:54:11, Fabrice wrote:
> > > We have to go through the navigator here because calling
> > > FrameTreeNode::DidStopLoading will not necessarily end up calling
> > > WebContentsImpl::DidStopLoading.
> > > That is also why we introduced the "page_start_loading_with_frame"
argument
> on
> > > the other CL.
> > 
> > I don't quite follow. During Navigate() we simulate DidStartLoading. If this
> is
> > FTN level operation, than it makes sense that undoing its work is calling
> > DidStopLoading on the FTN as well.
> > Why would we expect that WebContentsImpl::DidStopLoading is called for every
> > invocation of this code? Can't we have two sibling iframes start loading and
> > stop loading through those paths and we only need to do one call to
> > WebContentsImpl::DidStopLoading.
> 
> I think this is going to break some tests, trying it in next CL.

Different CL or a new patchset?

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

https://codereview.chromium.org/1080143003/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:362: // Notify the WebContents.
Simple comments that describe behavior of code that is obvious are not allowed
by the style guide.

[Charlie Reis, 2015-04-15 15:52:24]

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

https://codereview.chromium.org/1080143003/diff/20001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:362: // Notify the WebContents.
On 2015/04/15 15:39:28, nasko wrote:
> Simple comments that describe behavior of code that is obvious are not allowed
> by the style guide.

Actually, I think this comment is useful.  It's not obvious that this notifies
the WebContents, since it looks like it's telling the Navigator.

[Fabrice (no longer in Chrome), 2015-04-15 17:46:04]

fdegans@chromium.org changed reviewers:
+ erikchen@chromium.org

[Fabrice (no longer in Chrome), 2015-04-15 17:46:05]

As discussed, adding an accessor for the delegate in the Navigator interface. I
left the comments indicating the WebContents is being notified as I feel it is
not directly obvious but I'll remove them if you feel very strongly against
them.

+erikchen: FYI, I'm moving your scoped trackers. Again.

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

https://codereview.chromium.org/1080143003/diff/1/content/browser/frame_host/...
content/browser/frame_host/render_frame_host_impl.cc:853:
frame_tree_node_->navigator()->DidStopLoading();
On 2015/04/15 15:39:28, nasko wrote:
> On 2015/04/15 15:15:03, Fabrice wrote:
> > On 2015/04/14 19:54:41, nasko wrote:
> > > On 2015/04/14 16:54:11, Fabrice wrote:
> > > > We have to go through the navigator here because calling
> > > > FrameTreeNode::DidStopLoading will not necessarily end up calling
> > > > WebContentsImpl::DidStopLoading.
> > > > That is also why we introduced the "page_start_loading_with_frame"
> argument
> > on
> > > > the other CL.
> > > 
> > > I don't quite follow. During Navigate() we simulate DidStartLoading. If
this
> > is
> > > FTN level operation, than it makes sense that undoing its work is calling
> > > DidStopLoading on the FTN as well.
> > > Why would we expect that WebContentsImpl::DidStopLoading is called for
every
> > > invocation of this code? Can't we have two sibling iframes start loading
and
> > > stop loading through those paths and we only need to do one call to
> > > WebContentsImpl::DidStopLoading.
> > 
> > I think this is going to break some tests, trying it in next CL.
> 
> Different CL or a new patchset?

New patchset, sorry.

[nasko, 2015-04-15 19:17:06]

Thanks for bringing up the discussion and working through this! The CL is in
great shape and only a few comments (mostly minor) to resolve.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:200: // Notify the
RenderFrameHostManager of the event.
I wonder if it will be more consistent if all notifications are done after the
state on this FTN is changed to indicate it has started loading. If we don't,
for example the code in RFHM asking this FTN for HasStartedLoading will return
false, which will be contradictory.

The notification to WebContents is definitely gated on checking IsLoading(), so
that check must be done before changing the state of the FTN, but maybe that
result can be captured in a local variable and referenced later on.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:216:
render_manager()->OnDidStopLoading();
Same concern as above, should we change the state before notifying.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator.h:170: virtual NavigatorDelegate*
GetDelegate();
nit: I'd move this method right next to the GetController() one.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigator_delegate.h (right):

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator_delegate.h:113: virtual void
DidStopLoading() {}
Should this method take FTN as a parameter to be consistent with
DidStartLoading?

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

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1427: is_loading_ = true;
Why not set the state before calling the FTN method?

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1617: // done here either.
Thanks for improving this comment!

[Charlie Reis, 2015-04-15 23:37:53]

Yep, mostly nits from me.  Thanks!

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

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:366: double
FrameTree::GetLoadProgress() {
This should be in the .h file as load_progress() (since it's a one-liner that
can be inlined).

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:181: bool
FrameTreeNode::HasStartedLoading() const {
This should be in the .h file as has_started_loading().

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:185: void
FrameTreeNode::ResetLoadingProgress() {
Same: reset_loading_progress().

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:206:
frame_tree_->UpdateLoadProgress();
We should probably use DidChangeLoadProgress here rather than duplicating the
code.  (This will help if we later add another statement to this pattern.)

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:226:
frame_tree_->UpdateLoadProgress();
Same here: call DidChangeLoadProgress.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:132: bool HasStartedLoading()
const;
This is easily confused with IsLoading() above.  Can you clarify the difference
between them so that callers know which one to use?

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:137: // A RenderFrame in this node
started loading.
We're in the browser process, so let's say RenderFrameHost.  Same below.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/interstitial_page_navigator_impl.h (right):

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/interstitial_page_navigator_impl.h:26:
NavigatorDelegate* GetDelegate() override;
nit: Should come first.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator.h:170: virtual NavigatorDelegate*
GetDelegate();
On 2015/04/15 19:17:05, nasko wrote:
> nit: I'd move this method right next to the GetController() one.

+1, and I'd put it before GetController.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigator_delegate.h (right):

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator_delegate.h:104: // A RenderFrame in the
specified |frame_tree_node| started loading a new
nit: RenderFrameHost.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator_delegate.h:113: virtual void
DidStopLoading() {}
On 2015/04/15 19:17:05, nasko wrote:
> Should this method take FTN as a parameter to be consistent with
> DidStartLoading? 

I think it's probably ok to omit it until it's needed.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.h (right):

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.h:87: NavigatorDelegate* GetDelegate()
override;
Move above GetController.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1080143003/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3704: // current document.
Huh, this seems buggy to me, since it's not checking to_different_document. 
I'll ping dmazzoni about it.

(I'm wondering if we need FrameTreeNode at all in this method, or whether we can
do this BrowserAccessibilityManager stuff in RenderFrameHostImpl.  Or elsewhere,
as dmazzoni's TODO suggests.)

https://codereview.chromium.org/1080143003/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3741: // The change is
notified immediately for the first and last updates.
Let's rephrase slightly: something is getting notified of the change.  (The
change itself isn't notified.)

[Fabrice (no longer in Chrome), 2015-04-16 13:55:26]

Thanks for the comments all!
tl;dr for non-nits:
- is_loading in RFHI and the call to FTN need to be done in a certain order.
- Same for loading_progress in FTN and the call to WebContents.
- Moved the RFHM notification at the end of the method in both cases.
- Leave BrowserAccessibilityManager as is for now. We can move that in another
CL.

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

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree.cc:366: double
FrameTree::GetLoadProgress() {
On 2015/04/15 23:37:53, Charlie Reis wrote:
> This should be in the .h file as load_progress() (since it's a one-liner that
> can be inlined).

Done.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:181: bool
FrameTreeNode::HasStartedLoading() const {
On 2015/04/15 23:37:53, Charlie Reis wrote:
> This should be in the .h file as has_started_loading().

Then we'd have to move the kLoadingProgress constants back to the header, which
we didn't want to do.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:185: void
FrameTreeNode::ResetLoadingProgress() {
On 2015/04/15 23:37:53, Charlie Reis wrote:
> Same: reset_loading_progress().

Same as above.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:200: // Notify the
RenderFrameHostManager of the event.
On 2015/04/15 19:17:05, nasko wrote:
> I wonder if it will be more consistent if all notifications are done after the
> state on this FTN is changed to indicate it has started loading. If we don't,
> for example the code in RFHM asking this FTN for HasStartedLoading will return
> false, which will be contradictory.

I am not sure what you are referring to, afaict RFHM does not check the calling
FTN status, same for the WebContents. Tests assume calls
DidStartLoading/DidStopLoading and update of the load progress are done in a
specific order (see comments in the observer in
web_contents_impl_browsertest.cc) so that order cannot be changed.
On the other hand, the RFHM call seems to be independent so it can be done
before or after in both cases. I am changing it to be done after.

> The notification to WebContents is definitely gated on checking IsLoading(),
so
> that check must be done before changing the state of the FTN, but maybe that
> result can be captured in a local variable and referenced later on.

The is_loading tracking is done on the RFH. If we wanted to do that, we'd need
to pass an extra argument to FTN and WebContents, which we wanted to avoid.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:206:
frame_tree_->UpdateLoadProgress();
On 2015/04/15 23:37:53, Charlie Reis wrote:
> We should probably use DidChangeLoadProgress here rather than duplicating the
> code.  (This will help if we later add another statement to this pattern.)

Done.
Also updated the comments.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:216:
render_manager()->OnDidStopLoading();
On 2015/04/15 19:17:05, nasko wrote:
> Same concern as above, should we change the state before notifying.

Moved that at the end of the method.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:226:
frame_tree_->UpdateLoadProgress();
On 2015/04/15 23:37:53, Charlie Reis wrote:
> Same here: call DidChangeLoadProgress.

Done.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:132: bool HasStartedLoading()
const;
On 2015/04/15 23:37:53, Charlie Reis wrote:
> This is easily confused with IsLoading() above.  Can you clarify the
difference
> between them so that callers know which one to use?

I rephrased the comment. WDYT?

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:137: // A RenderFrame in this node
started loading.
On 2015/04/15 23:37:53, Charlie Reis wrote:
> We're in the browser process, so let's say RenderFrameHost.  Same below.

Done.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/interstitial_page_navigator_impl.h (right):

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/interstitial_page_navigator_impl.h:26:
NavigatorDelegate* GetDelegate() override;
On 2015/04/15 23:37:53, Charlie Reis wrote:
> nit: Should come first.

Done.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigator.h (right):

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator.h:170: virtual NavigatorDelegate*
GetDelegate();
On 2015/04/15 23:37:53, Charlie Reis wrote:
> On 2015/04/15 19:17:05, nasko wrote:
> > nit: I'd move this method right next to the GetController() one.
> 
> +1, and I'd put it before GetController.

Done.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigator_delegate.h (right):

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator_delegate.h:104: // A RenderFrame in the
specified |frame_tree_node| started loading a new
On 2015/04/15 23:37:53, Charlie Reis wrote:
> nit: RenderFrameHost.

Done.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator_delegate.h:113: virtual void
DidStopLoading() {}
On 2015/04/15 23:37:53, Charlie Reis wrote:
> On 2015/04/15 19:17:05, nasko wrote:
> > Should this method take FTN as a parameter to be consistent with
> > DidStartLoading? 
> 
> I think it's probably ok to omit it until it's needed.

I think the long term goal is to remove the FrameTreeNode from DidStartLoading
actually, if we remove the accessibility manager call. Semantically, we
shouldn't need to know about the FrameTreeNode at this point.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.h (right):

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.h:87: NavigatorDelegate* GetDelegate()
override;
On 2015/04/15 23:37:53, Charlie Reis wrote:
> Move above GetController.

Done.

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

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1427: is_loading_ = true;
On 2015/04/15 19:17:06, nasko wrote:
> Why not set the state before calling the FTN method?

In FTN, we're going to check the tree status to see if we started loading a
document, and then notify the WebContents. If we set is_loading_ to true too
early, the tree will always be in a loading state in FTN since at least this RFH
will be loading. So the order of these 2 statements matters here, as it does in
OnDidStopLoading.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/render_frame_host_impl.cc:1617: // done here either.
On 2015/04/15 19:17:06, nasko wrote:
> Thanks for improving this comment!

Welcome!

https://codereview.chromium.org/1080143003/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1080143003/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3704: // current document.
On 2015/04/15 23:37:53, Charlie Reis wrote:
> Huh, this seems buggy to me, since it's not checking to_different_document. 
> I'll ping dmazzoni about it.
> 
> (I'm wondering if we need FrameTreeNode at all in this method, or whether we
can
> do this BrowserAccessibilityManager stuff in RenderFrameHostImpl.  Or
elsewhere,
> as dmazzoni's TODO suggests.)

Maybe that's best left for another CL? We already do quite a lot in here.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3741: // The change is
notified immediately for the first and last updates.
On 2015/04/15 23:37:53, Charlie Reis wrote:
> Let's rephrase slightly: something is getting notified of the change.  (The
> change itself isn't notified.)

Done.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl_browsertest.cc (right):

https://codereview.chromium.org/1080143003/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:494:
EXPECT_TRUE(did_start_loading);
This implies DidStartLoading was called before the load progress was updated.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:502: EXPECT_EQ(0U,
progresses.size());
This implies there has been no loading progress update prior to calling
DidStartLoading.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl_browsertest.cc:509:
EXPECT_GE(progresses.size(), 1U);
This implies there has been at least one load progress update before calling
DidStopLoading.

[nasko, 2015-04-16 16:23:48]

LGTM with nits.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:181: bool
FrameTreeNode::HasStartedLoading() const {
On 2015/04/16 13:55:25, Fabrice wrote:
> On 2015/04/15 23:37:53, Charlie Reis wrote:
> > This should be in the .h file as has_started_loading().
> 
> Then we'd have to move the kLoadingProgress constants back to the header,
which
> we didn't want to do.

It still should be hacker_cased though, as it just returns a simple value
without doing any work.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:185: void
FrameTreeNode::ResetLoadingProgress() {
On 2015/04/16 13:55:25, Fabrice wrote:
> On 2015/04/15 23:37:53, Charlie Reis wrote:
> > Same: reset_loading_progress().
> 
> Same as above.

Again, hacker_cased.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:200: // Notify the
RenderFrameHostManager of the event.
On 2015/04/16 13:55:25, Fabrice wrote:
> On 2015/04/15 19:17:05, nasko wrote:
> > I wonder if it will be more consistent if all notifications are done after
the
> > state on this FTN is changed to indicate it has started loading. If we
don't,
> > for example the code in RFHM asking this FTN for HasStartedLoading will
return
> > false, which will be contradictory.
> 
> I am not sure what you are referring to, afaict RFHM does not check the
calling
> FTN status, same for the WebContents. Tests assume calls
> DidStartLoading/DidStopLoading and update of the load progress are done in a
> specific order (see comments in the observer in
> web_contents_impl_browsertest.cc) so that order cannot be changed.
> On the other hand, the RFHM call seems to be independent so it can be done
> before or after in both cases. I am changing it to be done after.
> 
> > The notification to WebContents is definitely gated on checking IsLoading(),
> so
> > that check must be done before changing the state of the FTN, but maybe that
> > result can be captured in a local variable and referenced later on.
> 
> The is_loading tracking is done on the RFH. If we wanted to do that, we'd need
> to pass an extra argument to FTN and WebContents, which we wanted to avoid.

My concerns are addressed by the usage of DidChangeLoadProgress earlier in the
method.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/navigator_delegate.h (right):

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/navigator_delegate.h:113: virtual void
DidStopLoading() {}
On 2015/04/16 13:55:25, Fabrice wrote:
> On 2015/04/15 23:37:53, Charlie Reis wrote:
> > On 2015/04/15 19:17:05, nasko wrote:
> > > Should this method take FTN as a parameter to be consistent with
> > > DidStartLoading? 
> > 
> > I think it's probably ok to omit it until it's needed.
> 
> I think the long term goal is to remove the FrameTreeNode from DidStartLoading
> actually, if we remove the accessibility manager call. Semantically, we
> shouldn't need to know about the FrameTreeNode at this point.

Acknowledged.

https://codereview.chromium.org/1080143003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1080143003/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:147: // |load_progress|. This will
notify the FrameTree who will in turn notify
nit: s/who/which/

[Fabrice (no longer in Chrome), 2015-04-16 16:33:30]

Thanks!
I'll send to CQ after rebasing on top of
https://codereview.chromium.org/1080073004/

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.cc (right):

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:181: bool
FrameTreeNode::HasStartedLoading() const {
On 2015/04/16 16:23:48, nasko wrote:
> On 2015/04/16 13:55:25, Fabrice wrote:
> > On 2015/04/15 23:37:53, Charlie Reis wrote:
> > > This should be in the .h file as has_started_loading().
> > 
> > Then we'd have to move the kLoadingProgress constants back to the header,
> which
> > we didn't want to do.
> 
> It still should be hacker_cased though, as it just returns a simple value
> without doing any work.

Done.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.cc:185: void
FrameTreeNode::ResetLoadingProgress() {
On 2015/04/16 16:23:48, nasko wrote:
> On 2015/04/16 13:55:25, Fabrice wrote:
> > On 2015/04/15 23:37:53, Charlie Reis wrote:
> > > Same: reset_loading_progress().
> > 
> > Same as above.
> 
> Again, hacker_cased.

Done.

https://codereview.chromium.org/1080143003/diff/60001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1080143003/diff/60001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:147: // |load_progress|. This will
notify the FrameTree who will in turn notify
On 2015/04/16 16:23:48, nasko wrote:
> nit: s/who/which/

Done.

[Charlie Reis, 2015-04-16 17:37:05]

LGTM

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
File content/browser/frame_host/frame_tree_node.h (right):

https://codereview.chromium.org/1080143003/diff/40001/content/browser/frame_h...
content/browser/frame_host/frame_tree_node.h:132: bool HasStartedLoading()
const;
On 2015/04/16 13:55:25, Fabrice wrote:
> On 2015/04/15 23:37:53, Charlie Reis wrote:
> > This is easily confused with IsLoading() above.  Can you clarify the
> difference
> > between them so that callers know which one to use?
> 
> I rephrased the comment. WDYT?

Sounds good, thanks.

https://codereview.chromium.org/1080143003/diff/40001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/1080143003/diff/40001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:3704: // current document.
On 2015/04/16 13:55:26, Fabrice wrote:
> On 2015/04/15 23:37:53, Charlie Reis wrote:
> > Huh, this seems buggy to me, since it's not checking to_different_document. 
> > I'll ping dmazzoni about it.
> > 
> > (I'm wondering if we need FrameTreeNode at all in this method, or whether we
> can
> > do this BrowserAccessibilityManager stuff in RenderFrameHostImpl.  Or
> elsewhere,
> > as dmazzoni's TODO suggests.)
> 
> Maybe that's best left for another CL? We already do quite a lot in here.

Yes, this will be separate.

[Fabrice (no longer in Chrome), 2015-04-16 21:55:35]

The CQ bit was checked by fdegans@chromium.org

[Fabrice (no longer in Chrome), 2015-04-16 21:55:35]

The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org,
creis@chromium.org
Link to the patchset: https://codereview.chromium.org/1080143003/#ps100001
(title: "Rebase")

[commit-bot: I haz the power, 2015-04-16 21:57:36]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1080143003/100001

[commit-bot: I haz the power, 2015-04-17 01:58:07]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-04-17 02:00:02]

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