pushState should not be treated as a navigation (step 1 of 3)

pushState should not be treated as a navigation (step 1 of 3)

Add a new version of RenderViewImpl::didStartLoading() that takes a boolean
indicating whether the navigation is to the same document or a different
document. blink will call this new version in a later patch.

BUG=50298
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243122

[jamesr, 2013-12-19 21:25:24]

I don't know enough about navigation to give quality reviews for patches like
this.

[Nate Chapin, 2013-12-19 21:27:39]

On 2013/12/19 21:25:24, jamesr wrote:
> I don't know enough about navigation to give quality reviews for patches like
> this.

Sorry about that, I reflexively filled out the name that the presubmit script
suggested.

[Charlie Reis, 2013-12-19 21:39:57]

I don't see a problem with this, but I'm curious what the larger plan is.  In
general, pushState should be treated as a navigation, right?  It creates a
navigation entry that you can go back/forward to.

Adding Nasko as a reviewer as well, since I'll be disappearing on leave soon and
he's currently looking at a pushState bug.  Might be good to chat with him about
your plan for steps 2 and 3.

[Nate Chapin, 2013-12-19 21:50:03]

On 2013/12/19 21:39:57, creis wrote:
> I don't see a problem with this, but I'm curious what the larger plan is.  In
> general, pushState should be treated as a navigation, right?  It creates a
> navigation entry that you can go back/forward to.
> 
> Adding Nasko as a reviewer as well, since I'll be disappearing on leave soon
and
> he's currently looking at a pushState bug.  Might be good to chat with him
about
> your plan for steps 2 and 3.

I think it depends on our definition of navigation. In most of the usual senses,
yes, pushState is a navigation. In the history state sense, definitely. In the
UI sense, no, and this is the case I'm trying to enable a fix for. I probably
could have come up with a better description :)

When didStartLoading is called, we can't tell whether it's same- or
different-document, so we do all the things we have to do on a
different-document navigation no matter what. Some of them (e.g., replacing
reload button with stop button and terminating fullscreen mode) are almost
certainly bugs in the case of a navigation within the current document.

Step 2 is to call the new didStartLoading from blink.

Step 3 is to delete the old didStartLoading.

(Step 4 is actually using the new bit, but I'm not sure I understand the
chromium-side well enough to actually plumb the bit to where it needs to go)

[Charlie Reis, 2013-12-20 00:02:36]

On 2013/12/19 21:50:03, Nate Chapin wrote:
> On 2013/12/19 21:39:57, creis wrote:
> > I don't see a problem with this, but I'm curious what the larger plan is. 
In
> > general, pushState should be treated as a navigation, right?  It creates a
> > navigation entry that you can go back/forward to.
> > 
> > Adding Nasko as a reviewer as well, since I'll be disappearing on leave soon
> and
> > he's currently looking at a pushState bug.  Might be good to chat with him
> about
> > your plan for steps 2 and 3.
> 
> I think it depends on our definition of navigation. In most of the usual
senses,
> yes, pushState is a navigation. In the history state sense, definitely. In the
> UI sense, no, and this is the case I'm trying to enable a fix for. I probably
> could have come up with a better description :)
> 
> When didStartLoading is called, we can't tell whether it's same- or
> different-document, so we do all the things we have to do on a
> different-document navigation no matter what. Some of them (e.g., replacing
> reload button with stop button and terminating fullscreen mode) are almost
> certainly bugs in the case of a navigation within the current document.
> 
> Step 2 is to call the new didStartLoading from blink.
> 
> Step 3 is to delete the old didStartLoading.
> 
> (Step 4 is actually using the new bit, but I'm not sure I understand the
> chromium-side well enough to actually plumb the bit to where it needs to go)

This will apply to fragment navigations and other in-page navigations as well,
right?

Is this similar to didNavigateWithinPage?  I guess
NavigationState::was_within_same_page isn't set early enough for that to work? 
(Just wondering if we need the new bit or if we already have that information.)

[Nate Chapin, 2013-12-20 00:14:30]

On 2013/12/20 00:02:36, creis wrote:
> On 2013/12/19 21:50:03, Nate Chapin wrote:
> > On 2013/12/19 21:39:57, creis wrote:
> > > I don't see a problem with this, but I'm curious what the larger plan is. 
> In
> > > general, pushState should be treated as a navigation, right?  It creates a
> > > navigation entry that you can go back/forward to.
> > > 
> > > Adding Nasko as a reviewer as well, since I'll be disappearing on leave
soon
> > and
> > > he's currently looking at a pushState bug.  Might be good to chat with him
> > about
> > > your plan for steps 2 and 3.
> > 
> > I think it depends on our definition of navigation. In most of the usual
> senses,
> > yes, pushState is a navigation. In the history state sense, definitely. In
the
> > UI sense, no, and this is the case I'm trying to enable a fix for. I
probably
> > could have come up with a better description :)
> > 
> > When didStartLoading is called, we can't tell whether it's same- or
> > different-document, so we do all the things we have to do on a
> > different-document navigation no matter what. Some of them (e.g., replacing
> > reload button with stop button and terminating fullscreen mode) are almost
> > certainly bugs in the case of a navigation within the current document.
> > 
> > Step 2 is to call the new didStartLoading from blink.
> > 
> > Step 3 is to delete the old didStartLoading.
> > 
> > (Step 4 is actually using the new bit, but I'm not sure I understand the
> > chromium-side well enough to actually plumb the bit to where it needs to go)
> 
> This will apply to fragment navigations and other in-page navigations as well,
> right?
> 
> Is this similar to didNavigateWithinPage?  I guess
> NavigationState::was_within_same_page isn't set early enough for that to work?

> (Just wondering if we need the new bit or if we already have that
information.)

Yeah, I think this is a case where we send the information eventually, but not
in time for the didStartLoading call.

[Charlie Reis, 2013-12-20 00:17:16]

On 2013/12/20 00:14:30, Nate Chapin wrote:
> On 2013/12/20 00:02:36, creis wrote:
> > On 2013/12/19 21:50:03, Nate Chapin wrote:
> > > On 2013/12/19 21:39:57, creis wrote:
> > > > I don't see a problem with this, but I'm curious what the larger plan
is. 
> > In
> > > > general, pushState should be treated as a navigation, right?  It creates
a
> > > > navigation entry that you can go back/forward to.
> > > > 
> > > > Adding Nasko as a reviewer as well, since I'll be disappearing on leave
> soon
> > > and
> > > > he's currently looking at a pushState bug.  Might be good to chat with
him
> > > about
> > > > your plan for steps 2 and 3.
> > > 
> > > I think it depends on our definition of navigation. In most of the usual
> > senses,
> > > yes, pushState is a navigation. In the history state sense, definitely. In
> the
> > > UI sense, no, and this is the case I'm trying to enable a fix for. I
> probably
> > > could have come up with a better description :)
> > > 
> > > When didStartLoading is called, we can't tell whether it's same- or
> > > different-document, so we do all the things we have to do on a
> > > different-document navigation no matter what. Some of them (e.g.,
replacing
> > > reload button with stop button and terminating fullscreen mode) are almost
> > > certainly bugs in the case of a navigation within the current document.
> > > 
> > > Step 2 is to call the new didStartLoading from blink.
> > > 
> > > Step 3 is to delete the old didStartLoading.
> > > 
> > > (Step 4 is actually using the new bit, but I'm not sure I understand the
> > > chromium-side well enough to actually plumb the bit to where it needs to
go)
> > 
> > This will apply to fragment navigations and other in-page navigations as
well,
> > right?
> > 
> > Is this similar to didNavigateWithinPage?  I guess
> > NavigationState::was_within_same_page isn't set early enough for that to
work?
> 
> > (Just wondering if we need the new bit or if we already have that
> information.)
> 
> Yeah, I think this is a case where we send the information eventually, but not
> in time for the didStartLoading call.

Ok, let's just be sure they match up in step 4, rather than introducing a new
concept.  (Mentioning for posterity in case someone else picks up the
content-side change.)  LGTM.

[commit-bot: I haz the power, 2013-12-20 18:15:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/japhet@chromium.org/102563003/1

[commit-bot: I haz the power, 2013-12-20 20:37:39]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2013-12-20 20:38:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/japhet@chromium.org/102563003/1

[commit-bot: I haz the power, 2013-12-21 00:46:23]

Retried try job too often on win_rel for step(s) unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-02 21:00:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/japhet@chromium.org/102563003/1

[commit-bot: I haz the power, 2014-01-02 22:12:22]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-02 22:14:07]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/japhet@chromium.org/102563003/1

[commit-bot: I haz the power, 2014-01-03 00:01:31]

Retried try job too often on win_rel for step(s) base_unittests, browser_tests,
interactive_ui_tests, net_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-03 21:32:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/japhet@chromium.org/102563003/1

[commit-bot: I haz the power, 2014-01-03 22:20:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/japhet@chromium.org/102563003/620001

[commit-bot: I haz the power, 2014-01-04 00:56:33]

Retried try job too often on win_rel for step(s) nacl_integration
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2014-01-06 17:18:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/japhet@chromium.org/102563003/620001

[commit-bot: I haz the power, 2014-01-06 18:21:45]

Change committed as 243122