738762 - Pass redirect and error information to global history

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[Marco Bonardo [:mak]]

We'd like to improve autocomplete results by removing some unwanted noise caused by redirects and error pages.  Though, to do so, the docshell should give us some more information about the visit.

Comment 1

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Not sure about which kind of tests to provide.  I can surely do some test involving Places, not testing the generic IHistory interface, but those should probably not live in the docshell test harness.
Would it be fine to just add a couple tests in Places test folders?

Comment 2

[Marco Bonardo [:mak]]

Attachment: patch v1.1

Corrected some indentation.  Trying with bz, I'm adding tests for history behavior in bug 737841, if you have ideas on how I may test this on the docshell side, I may try that.  Btw, the changes should be quite safe, I'm just adding a couple flags to an existing param, so history implementation tests may be enough.

Comment 3

[Marco Bonardo [:mak]]

Torisugari-san suggested we may rather distinguish between unrecoverable or recoverable errors, where history will then provide autocompletion for recoverable ones, not for the other ones.

So he suggests to do the following special cases:

(In reply to O. Atsushi (Torisugari) from comment #15)
> I think we should block:
> 501 Not Implemented             This is unlikely to fixed in the future.
> 505 HTTP Version Not Supported  This is unlikely to fixed in the future.
> 
> should not block:
> 408 Request Timeout             Retry will work.
> 413 Request Entity Too Large    Retry will work.
> (429 Too Many Requests)         Retry will work.

Imho we should not overengineer this too much, since most of these cases are unlikely to be hit often (some may probably never be hit in reality).
Out of the 4XX errors, I think 413 and 429 are unlikely to be fixed by a simple retry, so the only error we may consider "recoverable" imo is 408 (something broken in the connection).

So the condition could become:
if 400:500 - 408 + 501 + 505: UNRECOVERABLE_ERROR

Comment 4

[Marco Bonardo [:mak]]

PS: the above discussion comes from bug 737841

Comment 5

[Marco Bonardo [:mak]]

Attachment: patch v1.2

unbitrot, plus implements the above.

Comment 6

[Marco Bonardo [:mak]]

Comment on attachment 610270 [details] [diff] [review]
patch v1.2

Review of attachment 610270 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/nsDocShell.cpp
@@ +10640,5 @@
> +        // 408 is special cased, since may actually indicate a temporary
> +        // connection problem.
> +        else if (aResponseStatus != 408 &&
> +                 (aResponseStatus >= 400 && aResponseStatus <= 501 ||
> +                  aResponseStatus = 505)) {

ehr, should be ==, fixed locally

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 610270 [details] [diff] [review]
patch v1.2

r=me

Comment 8

[Marco Bonardo [:mak]]

The blocked bugs are pending review, since this is independent just pushed it, with the == fixed:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1001d5a9f57a

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1001d5a9f57a