Last Comment Bug 738762 - Pass redirect and error information to global history
: Pass redirect and error information to global history
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Marco Bonardo [::mak]
: Andrew Overholt [:overholt]
Depends on:
Blocks: 737836 737841
  Show dependency treegraph
Reported: 2012-03-23 12:55 PDT by Marco Bonardo [::mak]
Modified: 2012-04-11 02:33 PDT (History)
4 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch v1.0 (7.70 KB, patch)
2012-03-23 13:24 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
patch v1.1 (8.04 KB, patch)
2012-03-24 09:05 PDT, Marco Bonardo [::mak]
no flags Details | Diff | Splinter Review
patch v1.2 (8.36 KB, patch)
2012-03-28 13:20 PDT, Marco Bonardo [::mak]
bzbarsky: review+
Details | Diff | Splinter Review

Description Marco Bonardo [::mak] 2012-03-23 12:55:08 PDT
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] 2012-03-23 13:24:04 PDT
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?
Comment 2 Marco Bonardo [::mak] 2012-03-24 09:05:05 PDT
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.
Comment 3 Marco Bonardo [::mak] 2012-03-28 12:36:54 PDT
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] 2012-03-28 12:37:38 PDT
PS: the above discussion comes from bug 737841
Comment 5 Marco Bonardo [::mak] 2012-03-28 13:20:03 PDT
Created attachment 610270 [details] [diff] [review]
patch v1.2

unbitrot, plus implements the above.
Comment 6 Marco Bonardo [::mak] 2012-03-28 13:21:12 PDT
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 [:bz] (still a bit busy) 2012-03-28 22:09:15 PDT
Comment on attachment 610270 [details] [diff] [review]
patch v1.2

Comment 8 Marco Bonardo [::mak] 2012-03-29 06:09:03 PDT
The blocked bugs are pending review, since this is independent just pushed it, with the == fixed:
Comment 9 Ed Morley [:emorley] 2012-03-30 13:09:01 PDT

Note You need to log in before you can comment on or make changes to this bug.