Update a comment with a link to a historic source.

Verify that the page transition matches the navigation type with RendererDidNavigate[New|Auto]Subframe.

BUG=none
TEST=should all stay green

Committed: https://crrev.com/9f07a0c691b21d276cd74fbd0755bcbb81386430
Cr-Commit-Position: refs/heads/master@{#316905}

[Avi (use Gerrit), 2015-01-15 21:12:32]

avi@chromium.org changed reviewers:
+ creis@chromium.org

[Avi (use Gerrit), 2015-01-15 21:12:33]

This is a weird case, and I want to put a link to it directly in the code.

[Charlie Reis, 2015-01-15 22:12:09]

https://codereview.chromium.org/814563004/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/814563004/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigation_controller_impl.cc:1220: // navigation but
say "never mind" based on a flag from the renderer.
Maybe I misunderstood this case before, but I thought it was used for the first
navigation in a newly created iframe, which meant it shouldn't create a new
NavigationEntry.  That seems like a pretty standard case to me.

If so, I'm fine with updating the comment to be a bit more explicit about that,
though.

https://codereview.chromium.org/814563004/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigation_controller_impl.cc:1222: // This dates
back to http://crrev.com/115919 and the handling of immediate
This revision is a DEPS roll.  Did you mean http://crrev.com/18373 (from
https://codereview.chromium.org/115919)?

That CL is indeed weird and troubling, but thankfully it doesn't look like most
of its time-based ignore-redirect logic is still around.

[Avi (use Gerrit), 2015-01-15 22:16:52]

https://codereview.chromium.org/814563004/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/814563004/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigation_controller_impl.cc:1220: // navigation but
say "never mind" based on a flag from the renderer.
On 2015/01/15 22:12:09, Charlie Reis wrote:
> Maybe I misunderstood this case before, but I thought it was used for the
first
> navigation in a newly created iframe, which meant it shouldn't create a new
> NavigationEntry.  That seems like a pretty standard case to me.
> 
> If so, I'm fine with updating the comment to be a bit more explicit about
that,
> though.

Will investigate.

https://codereview.chromium.org/814563004/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigation_controller_impl.cc:1222: // This dates
back to http://crrev.com/115919 and the handling of immediate
On 2015/01/15 22:12:09, Charlie Reis wrote:
> This revision is a DEPS roll.  Did you mean http://crrev.com/18373 (from
> https://codereview.chromium.org/115919)

Yes. I used the auto-shortener and am bummed that it screwed up the URL.

> That CL is indeed weird and troubling, but thankfully it doesn't look like
most
> of its time-based ignore-redirect logic is still around.

Yeah.

[Avi (use Gerrit), 2015-01-16 21:06:26]

https://codereview.chromium.org/814563004/diff/1/content/browser/frame_host/n...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/814563004/diff/1/content/browser/frame_host/n...
content/browser/frame_host/navigation_controller_impl.cc:1220: // navigation but
say "never mind" based on a flag from the renderer.
On 2015/01/15 22:12:09, Charlie Reis wrote:
> I thought it was used for the first navigation in a newly created iframe

Isn't that what RendererDidNavigateAutoSubframe is for, though?

I'm rebuilding and looking at this.

[Avi (use Gerrit), 2015-01-16 23:03:21]

On 2015/01/16 21:06:26, Avi wrote:
>
https://codereview.chromium.org/814563004/diff/1/content/browser/frame_host/n...
> File content/browser/frame_host/navigation_controller_impl.cc (right):
> 
>
https://codereview.chromium.org/814563004/diff/1/content/browser/frame_host/n...
> content/browser/frame_host/navigation_controller_impl.cc:1220: // navigation
but
> say "never mind" based on a flag from the renderer.
> On 2015/01/15 22:12:09, Charlie Reis wrote:
> > I thought it was used for the first navigation in a newly created iframe
> 
> Isn't that what RendererDidNavigateAutoSubframe is for, though?
> 
> I'm rebuilding and looking at this.

I put in a NOTREACHED and this fails on all the new OOPIF tests. Sigh. But it
seems to hold for today's tests.

[Avi (use Gerrit), 2015-01-16 23:24:46]

On 2015/01/16 23:03:21, Avi wrote:
> On 2015/01/16 21:06:26, Avi wrote:
> >
>
https://codereview.chromium.org/814563004/diff/1/content/browser/frame_host/n...
> > File content/browser/frame_host/navigation_controller_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/814563004/diff/1/content/browser/frame_host/n...
> > content/browser/frame_host/navigation_controller_impl.cc:1220: // navigation
> but
> > say "never mind" based on a flag from the renderer.
> > On 2015/01/15 22:12:09, Charlie Reis wrote:
> > > I thought it was used for the first navigation in a newly created iframe
> > 
> > Isn't that what RendererDidNavigateAutoSubframe is for, though?
> > 
> > I'm rebuilding and looking at this.
> 
> I put in a NOTREACHED and this fails on all the new OOPIF tests. Sigh. But it
> seems to hold for today's tests.

BTW, this is at the WDYT stage, not looking for a review.

[Avi (use Gerrit), 2015-02-09 22:20:50]

https://codereview.chromium.org/814563004/diff/150001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/814563004/diff/150001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1265:
ui::PAGE_TRANSITION_AUTO_SUBFRAME));
Note that this fails on the reload tests because it comes through as
PAGE_TRANSITION_MANUAL_SUBFRAME. Do you think this is a bug?

[Charlie Reis, 2015-02-09 22:33:03]

https://codereview.chromium.org/814563004/diff/150001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl.cc (right):

https://codereview.chromium.org/814563004/diff/150001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl.cc:1265:
ui::PAGE_TRANSITION_AUTO_SUBFRAME));
On 2015/02/09 22:20:50, Avi wrote:
> Note that this fails on the reload tests because it comes through as
> PAGE_TRANSITION_MANUAL_SUBFRAME. Do you think this is a bug?

It certainly seems strange for this check to fail given the method name.  I'm
not entirely sure what the intention was, but it seems like things would be
easier to understand if we did enforce this check.

[Avi (use Gerrit), 2015-02-17 15:51:28]

Charlie, take a look. This cleans some things up, and I'd like to eventually
turn this into a straight DCHECK.

https://codereview.chromium.org/814563004/diff/190001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/814563004/diff/190001/content/renderer/render...
content/renderer/render_frame_impl.cc:3814: if (commit_type ==
blink::WebStandardCommit)
It turns out that I should have been looking at the value of commit_type this
whole time, and that using replacesCurrentHistoryItem was the wrong direction to
go.

[Charlie Reis, 2015-02-18 17:57:37]

Ok, I spent some time looking at PageTransition's AUTO vs MANUAL and
NavigationType's AUTO vs NEW.  I agree with you that we should never have
PT_AUTO + NT_NEW, and we'll aim to get rid of that from the OOPIF behavior soon.

LGTM, though it's probably worth adding a few extra tests (as noted) if
possible.

https://codereview.chromium.org/814563004/diff/190001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/814563004/diff/190001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:329: }
We should add a case for location.replace in the subframe, since we're removing
the replacesCurrentHistoryItem() call in RenderFrameImpl.  It's worth
demonstrating what we expect the correct behavior to be.

Can we also add a case for adding a subframe and show that it's PT_AUTO +
NT_NEW?  Or is that a problem because we don't have the frame to listen to?

https://codereview.chromium.org/814563004/diff/190001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/814563004/diff/190001/content/renderer/render...
content/renderer/render_frame_impl.cc:3814: if (commit_type ==
blink::WebStandardCommit)
On 2015/02/17 15:51:28, Avi wrote:
> It turns out that I should have been looking at the value of commit_type this
> whole time, and that using replacesCurrentHistoryItem was the wrong direction
to
> go.

Ok.  What's an example that shows the difference?  I'm not sure I understand
yet.

[Avi (use Gerrit), 2015-02-18 18:33:12]

https://codereview.chromium.org/814563004/diff/190001/content/browser/frame_h...
File content/browser/frame_host/navigation_controller_impl_browsertest.cc
(right):

https://codereview.chromium.org/814563004/diff/190001/content/browser/frame_h...
content/browser/frame_host/navigation_controller_impl_browsertest.cc:329: }
On 2015/02/18 17:57:37, Charlie Reis wrote:
> We should add a case for location.replace in the subframe, since we're
removing
> the replacesCurrentHistoryItem() call in RenderFrameImpl.  It's worth
> demonstrating what we expect the correct behavior to be.
> 
> Can we also add a case for adding a subframe and show that it's PT_AUTO +
> NT_NEW?  Or is that a problem because we don't have the frame to listen to?

More tests are always awesome. Let me try to make as many as I can, and I'll
follow up with you if need be.

https://codereview.chromium.org/814563004/diff/190001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/814563004/diff/190001/content/renderer/render...
content/renderer/render_frame_impl.cc:3814: if (commit_type ==
blink::WebStandardCommit)
On 2015/02/18 17:57:37, Charlie Reis wrote:
> On 2015/02/17 15:51:28, Avi wrote:
> > It turns out that I should have been looking at the value of commit_type
this
> > whole time, and that using replacesCurrentHistoryItem was the wrong
direction
> to
> > go.
> 
> Ok.  What's an example that shows the difference?  I'm not sure I understand
> yet.

The reload. That's a blink::WebHistoryInertCommit which is neither a history
navigation (WebBackForwardCommit) nor replaces the current history item. When I
was first writing this if() clause, it was "if replaces history item" which was
wrong, and I changed it to "if back/forward or replaces" when I should have
forgotten the "replaces history item" and straight-up checked the commit type.

[Charlie Reis, 2015-02-18 18:45:04]

https://codereview.chromium.org/814563004/diff/190001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/814563004/diff/190001/content/renderer/render...
content/renderer/render_frame_impl.cc:3814: if (commit_type ==
blink::WebStandardCommit)
On 2015/02/18 18:33:12, Avi wrote:
> On 2015/02/18 17:57:37, Charlie Reis wrote:
> > On 2015/02/17 15:51:28, Avi wrote:
> > > It turns out that I should have been looking at the value of commit_type
> this
> > > whole time, and that using replacesCurrentHistoryItem was the wrong
> direction
> > to
> > > go.
> > 
> > Ok.  What's an example that shows the difference?  I'm not sure I understand
> > yet.
> 
> The reload. That's a blink::WebHistoryInertCommit which is neither a history
> navigation (WebBackForwardCommit) nor replaces the current history item. When
I
> was first writing this if() clause, it was "if replaces history item" which
was
> wrong, and I changed it to "if back/forward or replaces" when I should have
> forgotten the "replaces history item" and straight-up checked the commit type.

Acknowledged.

[Avi (use Gerrit), 2015-02-18 20:59:56]

New patchsets have been uploaded after l-g-t-m from creis@chromium.org

[Avi (use Gerrit), 2015-02-18 21:01:20]

avi@chromium.org changed reviewers:
+ andrewhayden@chromium.org

[Avi (use Gerrit), 2015-02-18 21:01:20]

Andrew, can you stamp the translate manager file?

Charlie, here are the tests you asked for.

[Charlie Reis, 2015-02-18 21:06:44]

Thanks for the extra tests!  LGTM.

[Nico, 2015-02-18 22:28:07]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-02-18 22:28:08]

chrome/ lgtm

[Avi (use Gerrit), 2015-02-18 22:28:43]

On 2015/02/18 22:28:08, Nico wrote:
> chrome/ lgtm

I asked Nico to stamp because all the owners of translation are overseas from
me.

[Avi (use Gerrit), 2015-02-18 22:28:52]

The CQ bit was checked by avi@chromium.org

[commit-bot: I haz the power, 2015-02-18 22:30:06]

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

[commit-bot: I haz the power, 2015-02-18 22:51:46]

Committed patchset #12 (id:210001)

[commit-bot: I haz the power, 2015-02-18 22:52:35]

Patchset 12 (id:??) landed as
https://crrev.com/9f07a0c691b21d276cd74fbd0755bcbb81386430
Cr-Commit-Position: refs/heads/master@{#316905}