Consider an immediate redirect as machine-initiated and slow one as...

Consider an immediate redirect as machine-initiated and slow one as
user-initiated in maintaining navigation entries.
Also, ignore redirect- or machine-initiated- new subframe navigations.

The current code treats all redirects as machine-initiated in processing
navigation to a new page (to fix Bugs 9663 and 10531). This is not always
appropriate, because some sites, e.g., www.google.com/ig, use redirect to
implement user-initiated navigation (Bug 11896).

This change assumes that a machine-initiated redirect happens within 300ms
since the last document load was completed, while  a user-initiated one
happens later.

This assumption is not always correct, e.g., a user may cause transition within
300ms. But I cannot think of any better ways to tell if a redirect is machine-
initiated or user-initiated.

I believe this change works good enough, at least better than the status quo.

TEST=Open http://www.hp.com and observe it redirects to
http://www.hp.com/#Product . Hit Back button and observe
the former URL is not visited. Open http://www.google.com/ig and
click tabs inside the page, and try hitting Back and Forward to see if the
navigation is right. Open http://www.google.com/codesearch, search for
something, click on a result item, and try hitting Back.

BUG=11896, 12820

[yuzo, 2009-05-29 10:00:55]

Hi, Brett,

Do you think you have time to review this?

I'm not sure if this is the best way to fix the bug.
Any suggestions would be appreciated.

Yuzo

[brettw, 2009-06-01 17:01:25]

"FrameDocumentLoaded" seems somewhat unclear to me. How about
ViewHostMsg_DocumentLoadedInFrame? The corresponding delegate function would be
DocumentLoadedInFrame.

LGTM otherwise. If you agree with this, feel free to check in the code with the
change so you don't have to wait for another whole day because of the time
difference.

Thanks for the nice patch! I'm very sorry I took so long to review it.

[yuzo, 2009-06-02 12:03:06]

Hi, Brett,

Thank you for the review.
I renamed the message and the functions as you suggested.

I have two issues:
- I'm not a committer. :)
- Now that the navigation controller is time-sensitive,
  the history tests become flaky.

I've tried several values for
- kMaxAutoNavigationTimeDelta
- timeouts in the test HTML files
but failed to remove the flakiness.

Hence I've disabled the tests and added a TODO comment to fix them
later.

Is this acceptable?

(If there is a way to "inject" the kMaxAutoNavigationTimeDelta
value from ui_test to chrome, I may be able to fix the flakiness.
Is there an easy way?)

Yuzo


On 2009/06/01 17:01:25, brettw wrote:
> "FrameDocumentLoaded" seems somewhat unclear to me. How about
> ViewHostMsg_DocumentLoadedInFrame? The corresponding delegate function would
be
> DocumentLoadedInFrame.
> 
> LGTM otherwise. If you agree with this, feel free to check in the code with
the
> change so you don't have to wait for another whole day because of the time
> difference.
> 
> Thanks for the nice patch! I'm very sorry I took so long to review it.

[brettw, 2009-06-02 23:16:00]

On 2009/06/02 12:03:06, yuzo wrote:
> Hi, Brett,
> 
> Thank you for the review.
> I renamed the message and the functions as you suggested.
> 
> I have two issues:
> - I'm not a committer. :)
> - Now that the navigation controller is time-sensitive,
>   the history tests become flaky.

That's a good point. There's also the case that if the network is slow and the
redirect takes a bit longer, you will again see it in the list.

I have an idea: TabContents::OnUserGesture is used by the download manager to
know that the user has clicked on something. This is to prevent many downloads
without user interaction that might be a sign of spamming.

What do you think about having a much longer timeout and using this function to
see if something has happened? When the user does something, we would stop
waiting, since they may have caused a navigation. This would both address the
slow network case, and I think it would be long enough to avoid flakyness of the
test.

[yuzo, 2009-06-03 09:29:07]

Hi, Brett,

Thank you for your comments.

Yes, OnUserGesture is what I wanted. :)
I changed the code as suggested.

I even think now we can remove the time interval
constraint. Can we just say:
- user gesture means user-initiated
- no user gesture means automatic
?

My concern is that the flakiness remains.
It doesn't seem to be time-related.
Sometimes HistoryTester.ConsiderFastRedirectAsAutomatic
fails (the transition was erroneously judged as user-initiated.)

I'll investigate this tomorrow.


On 2009/06/02 23:16:00, brettw wrote:
> On 2009/06/02 12:03:06, yuzo wrote:
> > Hi, Brett,
> > 
> > Thank you for the review.
> > I renamed the message and the functions as you suggested.
> > 
> > I have two issues:
> > - I'm not a committer. :)
> > - Now that the navigation controller is time-sensitive,
> >   the history tests become flaky.
> 
> That's a good point. There's also the case that if the network is slow and the
> redirect takes a bit longer, you will again see it in the list.
> 
> I have an idea: TabContents::OnUserGesture is used by the download manager to
> know that the user has clicked on something. This is to prevent many downloads
> without user interaction that might be a sign of spamming.
> 
> What do you think about having a much longer timeout and using this function
to
> see if something has happened? When the user does something, we would stop
> waiting, since they may have caused a navigation. This would both address the
> slow network case, and I think it would be long enough to avoid flakyness of
the
> test.

[brettw, 2009-06-03 17:02:39]

On 2009/06/03 09:29:07, yuzo wrote:
> Hi, Brett,
> 
> Thank you for your comments.
> 
> Yes, OnUserGesture is what I wanted. :)
> I changed the code as suggested.
> 
> I even think now we can remove the time interval
> constraint. Can we just say:
> - user gesture means user-initiated
> - no user gesture means automatic

I chatted with Scott about this, and we both think having a timeout is still a
good idea. Let's say the page automatically loads some other page after 30
seconds. The user would probably expect to be able to go "back" to the previous
page even if they haven't clicked.

Plus, the user gesture doesn't cover every possible thing that can happen. For
example, if the user can navigate without clicking (I've actually seen some
pages where you can navigate though the site just by moving the mouse), then
this won't allow you to go back.

I guess I would recommend setting a max timeout of 5 seconds or so to cover
these edge cases.

[jon, 2009-06-03 23:52:22]

We should consider removing the original CL while we work on a fix.  The current
situation is much worse than what we had before, IMHO.  Reverting will give us
time to think through the change without the pressure of the milestone 3
deadline.

[yuzo, 2009-06-05 02:00:37]

Sorry for the late response -- I was sick yesterday.

Brett, sorry for bothering you, but can you either:
- Revert http://codereview.chromium.org/100245
- or commit this change?

I'd recommend the latter, but won't object for the former.

I've changed the time limit to 5 sec, and changed the flaky test.
So I believe this change is good to go.

Yuzo

On 2009/06/03 23:52:22, jon wrote:
> We should consider removing the original CL while we work on a fix.  The
current
> situation is much worse than what we had before, IMHO.  Reverting will give us
> time to think through the change without the pressure of the milestone 3
> deadline.

[yuzo, 2009-06-05 04:17:03]

I hit another flakiness :-( .

I disabled the tests for now.

Yuzo

On 2009/06/05 02:00:37, yuzo wrote:
> Sorry for the late response -- I was sick yesterday.
> 
> Brett, sorry for bothering you, but can you either:
> - Revert http://codereview.chromium.org/100245
> - or commit this change?
> 
> I'd recommend the latter, but won't object for the former.
> 
> I've changed the time limit to 5 sec, and changed the flaky test.
> So I believe this change is good to go.
> 
> Yuzo
> 
> On 2009/06/03 23:52:22, jon wrote:
> > We should consider removing the original CL while we work on a fix.  The
> current
> > situation is much worse than what we had before, IMHO.  Reverting will give
us
> > time to think through the change without the pressure of the milestone 3
> > deadline.

[Mark Larson, 2009-06-10 22:16:59]

Is this progressing? It's a regression we want to get fixed so we can push a new
build to the Beta channel.

[brettw, 2009-06-10 22:24:02]

LGTM

[yuzo, 2009-06-15 08:21:50]

Submitted as r18373