Closed Bug 790305 Opened 7 years ago Closed 7 years ago

Don't add history entries for redirects or error pages

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 19
Tracking Status
firefox19 --- verified

People

(Reporter: wesj, Assigned: wesj)

Details

Attachments

(1 file, 1 obsolete file)

I see a lot of 404's or redirects in my history. I don't want/need them there. i.e. I have a bunch of pages with the title "Problem loading page" and the neterror favicon.

We currently seem to add anything we get in LocationChange. I wonder if we can check the state there first, or maybe delay adding the history entry until later?
Attached patch Patch (obsolete) — Splinter Review
This seems to work well. Visiting twitter.co used to add four entries (twitter.co, www.twitter.com, twitter.com, and mobile.twitter.com). With this it adds two (twitter.com which has a meta refresh and some js) which sends you to mobile.twitter.com (not sure how to filter the twitter.com address out?).

Loading facebook.com goes from three redirect entries to just one entry for m.facebook.com.

Invalid urls like aadvadf.adfadf or non-existant pages like dl.dropbox.com/u/72157/a don't appear.

We seem to update history a lot (to much?). Details: This makes us not update on location changes or additions to the session history (but it does update the UI in these cases). It modifies things to add an entry onStateChange if the load was a success.

It also changes our nsAndroidHistory.cpp implementation to not add entries if the they were a redirect or a failure. Note this has to differ from desktop where they store and filter on the transition type, allowing them to record but still ignore those visits.
Attachment #664606 - Flags: review?(mark.finkle)
Earlier today rnewman mentioned that some of his history entries have suspiciously high visit counts, perhaps caused by session restore. It looks like your change to handleSessionHistoryMessage might fix this, which would be nice.
Hmm... interesting. I'm not sure we need any of the updates in Tab.java to be honest. They seem mostly useful for updating the page titles we show in the Awesomescreen I guess. I could modify those calls so that they ONLY update, and don't create new entries.
I'm seeing typically 3 visits for each page that I visited once ever.

url                        title                                                                    date           visits
http://en.m.wikipedia.org  Hypotonia - Wikipedia, the free encyclopedia                             1348511771753  3

In fact, almost *every* page in my recent history that has never been visited on desktop looks like it has excess visits, typically 2 or 3. Here's the worst:

http://www.spanishdict.co  Silver in Spanish | English to Spanish Translation | Traductor español  1345736002493  84          -32         1512


I definitely haven't visited that page 84 times; once was enough! Note that it was last visited 32 days ago.
Wait, that last might be a URL collision - shared with other dictionary lookups. Still vaguely concerned by the lack of visits = 1 entries.
Comment on attachment 664606 [details] [diff] [review]
Patch

I need to understand more about each of these changes. I'm sure each has some affect and I'd like to know what we are really changing.
Attached patch Patch v2Splinter Review
This is a bit simpler, but removes all of our adding of entries in Tabs.java, since we're already adding them in nsAndroidHistory.cpp. It also leaves the code that disables adding entries that are error pages or redirects.
Attachment #664606 - Attachment is obsolete: true
Attachment #664606 - Flags: review?(mark.finkle)
Attachment #674297 - Flags: review?(mark.finkle)
Comment on attachment 674297 [details] [diff] [review]
Patch v2

Any way to add a test or two here?
Attachment #674297 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/8c8c0b8e97f8

Possible to test this?
Assignee: nobody → wjohnston
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
Very nice
Status: RESOLVED → VERIFIED
OS: Linux → Android
Hardware: x86 → ARM
You need to log in before you can comment on or make changes to this bug.