Closed
Bug 790305
Opened 12 years ago
Closed 12 years ago
Don't add history entries for redirects or error pages
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(firefox19 verified)
VERIFIED
FIXED
Firefox 19
Tracking | Status | |
---|---|---|
firefox19 | --- | verified |
People
(Reporter: wesj, Assigned: wesj)
Details
Attachments
(1 file, 1 obsolete file)
2.56 KB,
patch
|
mfinkle
:
review+
|
Details | Diff | Splinter Review |
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?
Assignee | ||
Comment 1•12 years ago
|
||
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)
Comment 2•12 years ago
|
||
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.
Assignee | ||
Comment 3•12 years ago
|
||
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.
Comment 4•12 years ago
|
||
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.
Comment 5•12 years 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 6•12 years ago
|
||
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.
Assignee | ||
Comment 7•12 years ago
|
||
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 8•12 years ago
|
||
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+
Assignee | ||
Comment 9•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c8c0b8e97f8
Comment 10•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8c8c0b8e97f8 Possible to test this?
Assignee: nobody → wjohnston
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
Comment 11•12 years ago
|
||
Very nice
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•