Pass redirect and error information to global history

RESOLVED FIXED in mozilla14

Status

()

Core
Document Navigation
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

unspecified
mozilla14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
(Assignee)

Comment 1

5 years ago
Created attachment 608847 [details] [diff] [review]
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?
(Assignee)

Comment 2

5 years ago
Created attachment 609004 [details] [diff] [review]
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.
Attachment #608847 - Attachment is obsolete: true
Attachment #609004 - Flags: review?(bzbarsky)
(Assignee)

Comment 3

5 years ago
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
(Assignee)

Comment 4

5 years ago
PS: the above discussion comes from bug 737841
(Assignee)

Comment 5

5 years ago
Created attachment 610270 [details] [diff] [review]
patch v1.2

unbitrot, plus implements the above.
Attachment #609004 - Attachment is obsolete: true
Attachment #609004 - Flags: review?(bzbarsky)
Attachment #610270 - Flags: review?(bzbarsky)
(Assignee)

Comment 6

5 years ago
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+
(Assignee)

Comment 8

5 years ago
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
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.