Closed Bug 738762 Opened 8 years ago Closed 8 years ago

Pass redirect and error information to global history

Categories

(Core :: Document Navigation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: mak, Assigned: mak)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch patch v1.0 (obsolete) — Splinter Review
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?
Attached patch patch v1.1 (obsolete) — Splinter Review
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.
Attachment #608847 - Attachment is obsolete: true
Attachment #609004 - Flags: review?(bzbarsky)
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
PS: the above discussion comes from bug 737841
Attached patch patch v1.2Splinter Review
unbitrot, plus implements the above.
Attachment #609004 - Attachment is obsolete: true
Attachment #609004 - Flags: review?(bzbarsky)
Attachment #610270 - Flags: review?(bzbarsky)
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 on attachment 610270 [details] [diff] [review]
patch v1.2

r=me
Attachment #610270 - Flags: review?(bzbarsky) → review+
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
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/1001d5a9f57a
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.