Last Comment Bug 639105 - A new entry does not appear in History menu for certain pages
: A new entry does not appear in History menu for certain pages
Status: VERIFIED FIXED
[places-next-wanted][fixed-in-places]
: regression
Product: Firefox
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: Firefox 6
Assigned To: Marco Bonardo [::mak] (Away 6-20 Aug)
:
Mentors:
http://www.dofus.com/en/mmorpg-game/o...
Depends on:
Blocks: 487777
  Show dependency treegraph
 
Reported: 2011-03-04 20:31 PST by Alice0775 White
Modified: 2011-05-13 05:01 PDT (History)
4 users (show)
mak77: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch v1.0 (11.87 KB, patch)
2011-04-29 05:36 PDT, Marco Bonardo [::mak] (Away 6-20 Aug)
dietrich: review+
Details | Diff | Splinter Review

Description Alice0775 White 2011-03-04 20:31:58 PST
Build Identifier:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b13pre) Gecko/20110304 Firefox/4.0b13pre ID:20110304030406

History entry does not show in History menu in MenuBar/FirefoxButton.

However, The entry appears in the Library/Sidebar

It works on Firefox3.5.x.
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.1.18pre) Gecko/20110304 Firefox/3.5.18pre ID:20110304030206

But not work on Firefox3.6.x and Minefield4.0b13pre.
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2.16pre) Gecko/20110304 Namoroka/3.6.16pre ID:20110304033304
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b13pre) Gecko/20110304 Firefox/4.0b13pre ID:20110304030406

Reproducible: Always

Steps to Reproduce:
1. Start Minefield with new profile
2. Open URL ( http://www.dofus.com/en/mmorpg-game/objects/2-items/1-amulet )
3. Alt > History or Click Firefox Button > History>

Actual Results:
 History entry does not show in History menu

Expected Results:
 History entry should show in History menu
Comment 1 Alice0775 White 2011-03-04 22:32:58 PST
Regression window:
Works:
http://hg.mozilla.org/mozilla-central/rev/b618ca270e9b
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090418 Namoroka/3.6a1pre ID:20090418030842
Fails:
http://hg.mozilla.org/mozilla-central/rev/907f2f562f44
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2a1pre) Gecko/20090419 Namoroka/3.6a1pre ID:20090419030935
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b618ca270e9b&tochange=907f2f562f44

In local build,
build from 55cb6c5991d1 : fail
build from c43a4464f76c : work
Regressed by:
55cb6c5991d1	Marco Bonardo — Bug 487777 - History menu is slow and takes ages to open, trunk version, r=dietrich
Comment 2 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-04-29 04:11:34 PDT
This page does a particular redirect like A -> B -> A that confuses the redirects code.
Comment 3 Alice0775 White 2011-04-29 04:34:48 PDT
Other case,
An extra history entry registered in sidebar/Library, but not history menu.

[STR]
1. Open http://t.co/cFm7yp4

[Actual]
I can see history entries as follows.

Sidebar/Library:
http://d.hatena.ne.jp/vividcode/20110425/1303740589
http://t.co/cFm7yp4  <-- extra?

History menu:
http://d.hatena.ne.jp/vividcode/20110425/1303740589

[Expected]
It must be the same,
Comment 4 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-04-29 04:44:12 PDT
Well, they should not always be the same, the history menu is a filtered view of the history sidebar, where only redirect targets are shown. If you think that entry in the sidebar is spurious (or completely useless), please file a bug apart.

The bug here is that the entry should NOT be filtered because it's a redirect target, and the history menu is missing functionality due to this.
Comment 5 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-04-29 05:36:20 PDT
Created attachment 529080 [details] [diff] [review]
patch v1.0

I suspect reviewing changes to this test is as painful as making them.
I added some comments, feel free to ask for more :)
Comment 6 Dietrich Ayala (:dietrich) 2011-05-02 13:45:26 PDT
Comment on attachment 529080 [details] [diff] [review]
patch v1.0

> 
>   if (mRedirectsMode == nsINavHistoryQueryOptions::REDIRECTS_MODE_SOURCE) {
>+    // Should exist at least one visit that is not a redirect target.

"At least one visit should exist that is not a redirect target."

or

"At least one visit that is not a redirect target should exist."

ditto for the other added comments like this.


r=me on the changes. my only concern is that we don't have performance testing that covers changes like this. can you do some query plan analysis, and testing on large history to get a sense of the perf risk with this?
Comment 7 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-05-02 13:50:44 PDT
Sure, will do some check, I think there is no interesting move here, since it's mostly a move from a NOT EXISTS to a EXISTS. But will test on my db and max db.
On the other side, having a faster query generally has low value if it's selecting wrong results.
Comment 8 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-05-03 06:00:47 PDT
The query plan is the same, the timing seems slightly better on new queries. the only drawback seems to be the redirect_source case where the full explain is slightly longer, but the timing doesn't change.
Comment 9 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-05-03 06:34:29 PDT
http://hg.mozilla.org/projects/places/rev/3e5d2b7831d0
Comment 10 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-05-11 05:03:10 PDT
http://hg.mozilla.org/mozilla-central/rev/3e5d2b7831d0
Comment 11 Simona B [:simonab] 2011-05-13 05:01:19 PDT
Verified on:
Mozilla/5.0 (X11; Linux i686; rv:6.0a1) Gecko/20110513 Firefox/6.0a1

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