53922 - Location bar down-arrow doesn't show URLs after being maxxed

Status: VERIFIED FIXED

(Core :: DOM: Navigation, defect, P1)

Description

[Ari Pollak]

(Note: I have no clue which Component this should fall under)
Using the Modern theme, After a certain number of URLs typed in the Location Bar
(I counted 15), the drop-down Location menu stops being updated.
For example, if there are already 15 addresses when you click the down-arrow at
the end of the Location bar, then you type in another URL, it does not show up
when you click the down-arrow again.

Comment 1

[Chris Waterson]

8 ball says, "Reply Hazy, Try XP Apps"

Comment 2

[Chris Waterson]

-> xpapps

Comment 3

[sairuh (rarely reading bugmail)]

->claudius

Comment 4

[Claudius Gayle]

I assume this dropdown menu is limited to 15 entries. You're saying that it is showing
only the first fifteen and not the most recent 15?

Radha, is this your widget right?

Comment 5

[Radha on family leave (not reading bugmail)]

Will take a look

Comment 6

[Claudius Gayle]

*** Bug 54948 has been marked as a duplicate of this bug. ***

Comment 7

[johng]

nav triage team:
since this is dataloss, P1, rtm need info
Our hope this is an easy fix, maybe just resort what we show on the frontend.
If it is a hard fix, we may have to live with this dataloss.

Comment 8

[Ari Pollak]

Just to clarify here, this happens with all chromes I try, even third-party ones
from resources.themes.org.

Comment 9

[Radha on family leave (not reading bugmail)]

Attachment: patch nsUrlbarHistory.cpp, nsIUrlbarHistory.cpp sessionHistoryUI.js

Comment 10

[Jean-Francois Ducarroz]

R=ducarroz for the new patch which corrects small problem. Not yet attached to 
this bug report

Comment 11

[Radha on family leave (not reading bugmail)]

Attachment: Newer patch that ducarroz reviewed.

Comment 12

[Radha on family leave (not reading bugmail)]

alecf has provided super review

Comment 13

[don]

PDT, we have the reviewed and approved fix.  Please "++" it.  Thanks.

Comment 14

[Jim Roskind]

This patch is Gigantic by the standards of what is being alowed into the branch
at this point.  I think this will have to be minused, but I'll wait to get
broader consensus at the PDT on Friday.
If folks want to argue, they are welcome to attend... but this feature seems
broken... and the code change does not appear to be acceptable at this point in
time.
Is there a smaller hack fix which would get 99% of the value??  IF so, add the
patch, or mark it need-info to stop the impending minus.

Comment 15

[Radha on family leave (not reading bugmail)]

Jim,

Maybe I should have added the following description to the bug, before it became 
rtm+. Please go thro' it, especially the RISKS part. Most of the changes have 
been fished out from earlier versions of the file and are dumb-ass array 
management code. No big change in logic. Hope this helps,

Problem Description: 
--------------------- 
The popup under the urlbar displays the first fifteen entries from the 
urlbar history instead of the last fifteen. This bug started appearing 
ever  since I added persistance to urlbar history. Unfortunately, when I 
implemented persistance using RDF localstore, I did not realise that RDF 
does not allow fetching of data from the localstore in the reverse order 
ie., once items are in the localstore, I can fetch only from the first 
to last, and not the other way. 

Solution: 
--------- 
The solution is to read in the entries from local store in to memory at startup 
and use the in-memory copy to create the menus. 

Changes: 
---------- 
The changes involve 

0) New interfaces to add urls and to fetch entries from the cache 
1) reading in all the contents of the localstore in to  nsVoidArray list at 
startup 
2) Adding entries to the cache copy (in addition  to RDF) whenever a new url is 
typed in to location bar 
3) Delete entries from the cache (in addition to  RDF)  whenever the history is 
cleared. 
4) Use the in-memory copy for autocompletion instead of reading from RDF. 
5) Removal of RDF based code from Javascript and move it to the component. 

I'm reading in all the values at startup so that the component will maintain 
consistency  whenever the browser is up and running. 

Please note that the new methods(AddEntry and GetEntryAtIndex along with 
cache) were present in earlier versions when persistance was not 
available. I have basically fished out the old code and integrated the 
it with current code that supports RDF 

Risks: 
------- 
Though the changes look little extensive, I believe the risks of this patch are 
not high. The changes are contained to just one module and there are not many 
clients to this module. Most of the new code was already there in previous 
versions. 

Alternate solutions considered: 
-------------------------------- 
I considered the alternate solution of reading in data from RDF at startup, 
maintaining just a cache thro' out the life time of the browser and writing back 
to RDF at shutdown. But  for some reason that didn't work. RDF wouldn't actually 
write the data to disk at shutdown though calls succeeded. So, I'm not pursuing 
that method at this time. 

Comment 16

[Blake Ross]

I would also suggest doing a -w patch (to ignore whitespace), and remove the 
comments, just for comparison -- I expect that you'll find a much shorter 
patch.  I see quite a few multiline comments and whitespace changes, especially 
near the end of the patch.

Comment 17

[Chris Waterson]

I think I have a simpler fix. Attaching now.

Comment 18

[Chris Waterson]

Attachment: simple fix: use RDF sequence to hold links in ordered collection

Comment 19

[Chris Waterson]

I'd like to re-open the floor on this bug. Radha's solution involves changing a
bunch of APIs, my solution involves changing the storage mechanism to take
advantage of RDF's ordered collection stuff. I've tested the patch above on
Linux, and it does in fact "roll off" URLs as expected. The only bug with it
that I can see (that already exists with the existing implementation) is that it
doesn't promote an existing URL to the top of the queue. I dunno what 4.x did,
maybe that's ok.

Comment 20

[Jean-Francois Ducarroz]

About the fact we are not promoting a URL to the top of the queue is to remove it then reinsert it in the queue. 

+    if (entries.IndexOf(rdf.GetLiteral(urlToAdd)) != -1) {
+      // we've got it already
+      //dump("URL already in urlbar history\n");
+      return;

At this point, do not return but instead, remove it from the queue. The code right after will reinsert it into the 
queue like if it was a brand new url.

Comment 21

[Jean-Francois Ducarroz]

Chris, your patch totally  broken urlbar autocompletion which use the same local store! You need to change also the 
code in xpfe/components/urlbarhistory/src. Also, you should not delete "fall off" entries has autocomplete need 
them.

Comment 22

[Radha on family leave (not reading bugmail)]

That is what I noticed. I will work on this after lunch.

Comment 23

[Chris Waterson]

Zoiks! I guess I didn't realize that this was used elsewhere. radha: do you
think this is a simpler way to proceed? (If not, what can I do to convince you?
*wink* *wink*)

It sounds like you're going to carry it from here: I'll be on IRC if you need
me, or feel free to kick the bug over to me if you'd like me to finish it up.

Comment 24

[Jean-Francois Ducarroz]

Chris, just for your info:

Both history dropdown list and autocomplete use the same data source
Both need to display most recent URL first
Only history dropwdown is limited to 15 most recent url

Comment 25

[Chris Waterson]

Is there an upper limit to the number of URLs that we'll use for autocompletion?
If not, there should be (say, 100). RDF is storing stuff as linked lists
internally, so this could get ugly if we don't bound the size.

Comment 26

[Jean-Francois Ducarroz]

Not as I know. It's up to urlbar history to fix the limite. Radha?

Comment 27

[Radha on family leave (not reading bugmail)]

Attachment: More diffs. (Not completely functional though)

Comment 28

[Radha on family leave (not reading bugmail)]

I have attached more diffs. But they don't work completely. I modified the 
component nsUrlbarHistory.cpp to use the nsIRDFContainerUtils and 
nsIRDFContainer methods for operations like ClearHistory(), GetCount() which are 
used when you go to prefs-> history panel and try to clear the history  But even 
though the calls to remove the elements from the datasource succeeds, the 
elements don't seem to be actually removed. So, you can clear the history, close 
the prefs window, open prefs again, go to history panel, notice that the 
ClearHistory button is still active. That means the call to 
nsUrlbarHistory->getCount() returns a value > 0. I went inside the debugger and 
saw that the loop iterates thro' as many elements that were there before I 
cleared it last. Instead of iterating thro' the datasource elements, if I call 
container->GetCount(), that also returns a value > 0 even though I had just 
cleared them all. So, there seem to be some sync up problem between the 
container and the actual datasource. Waterson, Is there any method to sync these 
two, so that the container and the datasource know each others state. 

In nsUrlbarHistory.cpp I also tried to completely ignore the container and 
directly go to the datasource for ClearHistory() and GetCount(). But that didn't 
work either. when I did that, even though there were elements in the datasource, 
hasMoreElements() returned false. 

Chris waterson, So, I'm kinda lost again. You can apply the attached patch to 
the trunk and see what I'm trying to say by putting breakpoints in 
nsUrlbarHistory.cpp->ClearHistory() and GetCount().

Comment 29

[Jim Roskind]

rtm need info.
This patch needs to settle down, and be reviewed, and then renominated for a
branch landing.

Comment 30

[Radha on family leave (not reading bugmail)]

we could probably put a limit.  I'm fine with a reasonable number. But   I also 
have a bug that says, link clicks in the browser should also get in to urlbar  
history. I believe 4.x does that. If I implement that, we may easily max out a 
limit like 100.

Comment 31

[Ben Bucksch (:BenB)]

> I also have a bug that says, link clicks in the browser should also get in to
> urlbar history.

WONTFIX. the urlbar history is for what I typed into the urlbar (and exactly
that IMO), not a global history.

> I believe 4.x does that.

It doesn't, at least not on Linux.

Comment 32

[Chris Waterson]

Attachment: massage previous patch to work; add limit to urlbarhistory items

Comment 33

[Chris Waterson]

radha & I massaged the previous set of patches to work, and added an upper limit
(100) to the number of entries that'll be stored in localstore.rdf.

Comment 34

[Chris Waterson]

alecf & ducarroz, could you look over these?

Comment 35

[Jean-Francois Ducarroz]

Waterson, I have some trouble to apply your last patch. The diff you did for xpfe/components/urlbarhistory/src/
nsUrlbarHistory.cpp against version 1.11 doesn't match at all the current version.1.11 of this file!

Comment 36

[Jean-Francois Ducarroz]

please ignore my last comment about the patch. Was a mistake...

Comment 37

[Alec Flett]

this looks good - I didn't know about MakeSeq either - pretty neat stuff.

minor nit - in sessionHistoryUI.js, you have

+          if (!na)
+             na = document.getElementById("nothing_available");                 

in both the if() {} and else {} - why not just put it outside the whole if 
statement (less code to maintain, and it took me a few seconds to realize that 
this DID NOT mean we were putting the "(Nothing Available)" in the URL bar when 
we actually had elements to put there)

Comment 38

[Jean-Francois Ducarroz]

I won't give you my review as I am not familar with RDF at all. I will let Radha
do that. However, your last patch seems to work very well and fix all the issues
we have either in urlbar history or in url autocomplete.

Now, urlbar history always show the most 15 recent URL, the last one at the top.
If you re-enter a existing url, it move to the top of the list.

And url autocompletion autocomplete against the last 100 url (I haven't tested
with more than 20), proposing by default the most recent one:

-visite www.mozilla.org
-visite www.mozillazine.org
-type www.mo and autocomplete will complete to www.mo[zillazine.org] and show
both url in the dropdown list
-visite again www.mozilla.org
-type www.mo an you get this time www.mo[zilla.org] and both url in the dropdown
list.

Very good fix.

Comment 39

[Chris Waterson]

alecf: agreed; good point.

jfd: FWIW, radha and I tested the limits by cranking down MAX_HISTORY_ITEMS (to
3) and MAX_HISTORY_MENU_ITEMS (to 6) and verified that this works as expected.

Comment 40

[Alec Flett]

great - oh and [s]r=alecf if you need a [super] reviewer to check in against..

Comment 41

[Claudius Gayle]

sooo, according to Jar's 10/13 comments now that the patch has settled down and been
(s)+(r)eviewed this fix is ready to get back on the ++ train right? Someone needs to rtm+ it
first though. Chopchop, let's go!

Comment 42

[Alec Flett]

since this was at one time +'ed I'm going to take the liberty of changing the 
status back to rtm+

Comment 43

[don]

Agreed.  This should be "rtm+" again.

Comment 44

[Radha on family leave (not reading bugmail)]

Attachment: Patch to fix the "nothing available"

Comment 45

[Radha on family leave (not reading bugmail)]

I have attached a patch that takes care of the "Nothing available" problem. 
Since the original hack didn't quite work out, I decided to create and add the 
"Nothing Available" menu item when there is nothing to show. I have tested 
it well in all combinations. No change to nsUrlbarHistory.cpp in the latest 
patch. I have attached it for completeness sake. Alec, ducarroz let me know if 
you can bless those js/xul changes. Otherwise, I will find ben.

Comment 46

[Claudius Gayle]

*** Bug 55176 has been marked as a duplicate of this bug. ***

Comment 47

[Alec Flett]

the difference is that "na" is now a string...sounds good sr=alecf

Comment 48

[Ben Bucksch (:BenB)]

Am I understanding it correctly that the pop-up menu will now come up, even if
there is no possible completion found? What sense does it make to display
"nothing available"?

Comment 49

[Alec Flett]

"nothing available" gives the user feedback. It lets them know that the URL bar
is not simply broken.

Comment 50

[Jean-Francois Ducarroz]

"nothing available" will appears only the URL History dropdown list (the one at the right of the URL field), it won't 
appears in autocompletion dropdown list.

Comment 51

[Jean-Francois Ducarroz]

R=ducarroz for Radha modification for "nothingAvailable" menu item.

Comment 52

[Peter Trudelle]

PDT says patch looks like too much risk for too little benefit this late in the
cycle. RTM-

Comment 53

[Ben Bucksch (:BenB)]

ops, I confused the bugs. Sorry. For the drop-down, this makes sense, of course.

Can we get this into the trunk, please?

Comment 54

[Phil Peterson]

Since radha and johng think it's really important to get this fix on the branch,
I suggested that she check into the trunk and get QA verification that the bug
is fixed, and that nothing else has regressed. Marking [rtm need info].

Comment 55

[Radha on family leave (not reading bugmail)]

PDT: I hope the following explanation clears your concern about the patch size:

Problem:
--------
The problem is about the urlbar history window showing the oldest entries and 
not the latest entries. You may have thought that the solution would be a simple 
one that just reverses the beginning and end point of a loop. But the problem is  
more complex than that. Here's why:

The menu is created out of entries gotten from RDF. The original code used 
simple RDF interfaces (nsIRDFDataSource) to add entries to RDF and get entries 
off of that. These interfaces do not allow inserting elements at the top, they 
only allow elements to be appended to the bottom. I could not get what I wanted 
ie., insert at top with the old code, so that when I query, I will get the 
latest entries first. 

Fix:
----
With the help of Chris Waterson, I reworked all the RDF calls in the component 
(cpp and JS files) to use nsIRDFContainerUtils  so that I can insert at the top 
and there by get the results I want. So if you look closely at the patch, 
primary changes are to remove the old calls to nsIRDFDataSource and add new code 
to use nsIRDFContainerUtils. 

The other way to fix this was to read in elements in to  a in-memory array at 
startup and use the in-memory array for all menu creation. This patch was 
proposed earlier on 10/11/00 (The second patch in the list above) and got 
rejected by PDT because that  was big too. I do not believe, I can solve this 
problem in any shorter patch.  (Waterson, please add in, if you think there are 
shorter ways)

The patch is contained to one component nsIUrlbarHistory and its associated JS 
file. The popup to the right of the urlbar and urlbar autocompletion are the 
only clients of this component.

Testing Done:
-------------
 I have tested the following things after coming up with the fix:

1) When the user types in a url in the urlbar, make sure elements are inserted 
at the top
2) Make sure the pulldown menu shows the most recently entered urls  and not the 
earlier urls. 
3) If a url that is about to be added to the history is already in the history, 
move it from its current position in the history to the top so that it will show 
up  in the menu when the user clicks on it. 
4) Make sure Prefs->Clear History actually clears the elements from RDF
6) Make sure that when there are no entries in the history, it shows the menu 
item "Nothing available".  This feature was already there. But since all the 
code around it changed, I had to make sure it continued to work. 
7) Open an additional browser window and make sure that  both the windows show 
the same thing when clicked on the urlbar history pulldown.
8) When autocompleting make sure ot  autocompletes with the most recently 
entered url first and then shows other older urls next. Please see ducarroz 
comments regarding this on 10/16/00 16:38 

In short I believe, this patch only makes the feature work better and does not 
regress any existing functionality. 

Please re-consider for rtm. Thanks!

Comment 56

[Radha on family leave (not reading bugmail)]

Fix already checked in to trunk

Comment 57

[Jason Eager]

this should get a rtm+, since the patch is already on the trunk.

Comment 58

[selmer (gone)]

rtm++

Comment 59

[Ben Bucksch (:BenB)]

I think, the fix for this bug caused critical bug 57246.

Comment 60

[msanz]

OK, I'm reading this late since I wasn't copied, but we cannot take a new string
now. The reason is that adding a new string will break the language packs that
are included in the US product, it is not only the localized product that is at
risk, is the US one as well.

Comment 61

[Jean-Francois Ducarroz]

I haven't seen any new string added in the fix! They are using existing one.

Comment 62

[Chris Waterson]

If this fix goes onto the branch, then we must also land the fix for bug 57246
on the branch.

Comment 63

[Jim Roskind]

Pushing back to rtm need info in light of the comment by Waterson and Ben B.
Bug 57246 seems (in its current incarnation) to have a relatively short and
understandable patch, and based on Waterson's statement, it is a required part
of landing this bug's patch.
I don't think that change would impact the risk assessment of this patch's
landing, and (assuming that patch is reviewed and approved) I'd suggest
augmenting the current patch in this bug with that patch, get it reviewed,
approved, and nominated for a rapid checkin.

Comment 64

[msanz]

Radha sent me a note saying that she needs to add a string definition to a
properties file. Is the string new in navigator.properties, so I guess she's
adding a string.

Comment 65

[Radha on family leave (not reading bugmail)]

msanz, if you look at the last patch in this bug, you will see a patch to 
xpfe/browser/resources/locale/en-US/navigator.properties (which has  a 
definition to a string called nothingAvailable). This string definition is 
currently in navigator.dtd. I will not be using the one in navigator.dtd, I will 
be using the one in navigator.properties. 

I don't believe a  new translation is required here. An existing string has 
been moved from the .dtd file to .properties file. Please comment if this can be 
done at this time.

Comment 66

[msanz]

Right, even if the translation exists, moving it to a different file will force
us to update the lang packs. adding Tao for confirmation. Tao, can you confirm
that this change would break the lang packs?

Comment 67

[Ben Bucksch (:BenB)]

radha, why don't you just use the string from the dtd anywhere hidden in
navigator.xul and then get the value from that node in JS? That way, you don't
have to make changes in /locale/.

Comment 68

[tao]

A lengthy bug report... Anyway, pls see my comment below.
>Right, even if the translation exists, moving it to a different file will force
>us to update the lang packs. adding Tao for confirmation.

There are two issues here:
1. While DTD files use UTF_8 encoding, property files use escape-unicode. Even
   we have the translation, L10n engineers will need to convert it.
2. Introducing new entities to DTD files will cause the client to crash.
   Introducing new key, value pair to properties might cause the client to
   malfunction (crash if return value is not handled properly...). The latter
   is what we are facing now.


> Tao, can you confirm
>that this change would break the lang packs?

Yes, this change will cause the langpack to break unless L10n engineers
manually update the langpacks.

Comment 69

[msanz]

And that's the problem, we cannot update the packages again.

Comment 70

[Radha on family leave (not reading bugmail)]

I have a patch that implements Ben Bucksch's suggestion above and eliminates the 
need for change to navigator.properties. I have sent it to Ben Goodger for 
review. I will attach it after clearing it with him.

Comment 71

[Radha on family leave (not reading bugmail)]

*** Bug 57723 has been marked as a duplicate of this bug. ***

Comment 72

[tao]

Hi, Radha:


Has this bug been reviewed & super-reviewed? Your latest patch seems to remove
the dependency on l10n group.

Comment 73

[Radha on family leave (not reading bugmail)]

Attachment: diffs with no dependency on lang-packs

Comment 74

[Radha on family leave (not reading bugmail)]

I have attached a patch that removes the dependency on the language packs. ie., 
no changes to navigator.properties file. This patch goes on top of the existing 
patch on trunk. This works on mozilla and commercial builds. I haven't received 
a feedback from Ben yet for the use of broadcasters to create a dummy node.  If 
anyone knows of a better method to create a dummy node that will hold a id and 
value(and does not  have UI attached to it), please update me. However the 
current patch works just fine.  

Comment 75

[Ben Bucksch (:BenB)]

> I haven't received
> a feedback from Ben yet for the use of broadcasters to create a dummy node.

You mean me? I don't know, but I'd just create a hidden <box>.

Comment 76

[Radha on family leave (not reading bugmail)]

sorry, I meant Ben Goodger

Comment 77

[Radha on family leave (not reading bugmail)]

Hyatt has reviewed the latest patch with no change to the properties file. Can I 
get a super review? Please note that the latest changes to with the xul file 
with a dummy node will go in to the  branch  only. I intend to keep the 
properties file change in the trunk.  I 

Comment 78

[Alec Flett]

yay, good to see this sr=alecf

Comment 79

[Phil Peterson]

Marking rtm+ to get on today's PDT radar

Comment 80

[Radha on family leave (not reading bugmail)]

Attachment: Consolidated patch to branch

Comment 81

[Radha on family leave (not reading bugmail)]

As per JAR's comments earlier, I have attached a consolidated patch that will go
in to the branch. It also has the patch to bug 57246(a regression caused by this
bug in RDF). 57246 has been fixed on the trunk. So is this one. No change to UI,
lang-packs  required. Thanks,

Comment 82

[selmer (gone)]

Did the Hyatt and AlecF reviews apply to the 10/24/00 15:47 patch?

Comment 83

[selmer (gone)]

This bug is in candidate limbo.  We will take this fix once we have a 
candidate in hand, but we can't take this fix before then.

Comment 84

[Radha on family leave (not reading bugmail)]

Yes, Hyatt's and Alec's reviews apply to the changes in the latest consolidated 
patch. Hyatt's review was provided by mail. Since there had been so many back 
and forths on this bug, I just put together everything in one set so that PDT 
(and I) will know what will go in to the branch. Thanks,

Comment 85

[selmer (gone)]

We are moving toward the candidate.  Please check this fix into the trunk so we 
can get get some cook time.

Comment 86

[Radha on family leave (not reading bugmail)]

99% of the latest patch attached is already in the trunk. The latest round of
reviews by Alec and Hyatt are for 2 line hacks in navigator.XUL and navigator.JS
 to avoid changes to lang-packs in the branch. These need not be checked in to
trunk, since trunk can accept changes to lang-packs. 

Comment 87

[Phil Peterson]

PDT marking [rtm++]. This bug is now out of limbo and approved for checkin to
the branch. Please check in ASAP.

Comment 88

[Radha on family leave (not reading bugmail)]

Fix checked in to the branch too.

Comment 89

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Verified fixed on branch with commercial release build 2000-10-31-14. Adding
vtrunk keyword.

Comment 90

[Asa Dotzler [:asa]]

This looks Fixed on 122604 win32 mozilla build on NT, 122610 mozilla linux build
on RedHat6.2 and 122609 mozilla mac build on OS9.  Removing vtrunk keywrod and
setting bug status to Verified.