Closed Bug 53922 Opened 24 years ago Closed 24 years ago

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

Categories

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

Product:
Core
Component:
DOM: Navigation
Platform:
x86
Linux
Type:
defect

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: ajp+mozilla, Assigned: radha)

References

Details

(Keywords: dataloss, Whiteboard: [rtm++] fix in hand. Fix on trunk)

Attachments

(8 files)

patch nsUrlbarHistory.cpp, nsIUrlbarHistory.cpp sessionHistoryUI.js
14.13 KB, patch
Details | Diff | Splinter Review
Newer patch that ducarroz reviewed.
15.10 KB, patch
Details | Diff | Splinter Review
simple fix: use RDF sequence to hold links in ordered collection
3.01 KB, patch
Details | Diff | Splinter Review
More diffs. (Not completely functional though)
8.51 KB, patch
Details | Diff | Splinter Review
massage previous patch to work; add limit to urlbarhistory items
9.94 KB, patch
Details | Diff | Splinter Review
Patch to fix the "nothing available"
11.87 KB, patch
Details | Diff | Splinter Review
diffs with no dependency on lang-packs
1.96 KB, patch
Details | Diff | Splinter Review
Consolidated patch to branch
12.40 KB, patch
Details | Diff | Splinter Review 
(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.
8 ball says, "Reply Hazy, Try XP Apps"
Component: XP Miscellany → XP Apps: GUI Features
-> xpapps
Assignee: waterson → ben
QA Contact: brendan → sairuh
->claudius
QA Contact: sairuh → claudius
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?
Assignee: ben → radha
Component: XP Apps: GUI Features → History
Keywords: rtm
Will take a look
*** Bug 54948 has been marked as a duplicate of this bug. ***
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.
Keywords: dataloss
Priority: P3 → P1
Whiteboard: [rtm need info]
Just to clarify here, this happens with all chromes I try, even third-party ones
from resources.themes.org.
Status: NEW → ASSIGNED
Whiteboard: [rtm need info] → [rtm need info] fix in hand. awaiting review
R=ducarroz for the new patch which corrects small problem. Not yet attached to 
this bug report
alecf has provided super review
PDT, we have the reviewed and approved fix.  Please "++" it.  Thanks.
Whiteboard: [rtm need info] fix in hand. awaiting review → [rtm+] fix in hand. reviewed and approved
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.
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. 
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.
I think I have a simpler fix. Attaching now.
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.
Whiteboard: [rtm+] fix in hand. reviewed and approved → [rtm+] fix in hand.
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.

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.

That is what I noticed. I will work on this after lunch.
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.
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

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.
Not as I know. It's up to urlbar history to fix the limite. Radha?
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().


rtm need info.
This patch needs to settle down, and be reviewed, and then renominated for a
branch landing.
Whiteboard: [rtm+] fix in hand. → [rtm need info] fix in hand.
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.
> 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.

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.
alecf & ducarroz, could you look over these?
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!

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

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)
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.

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.
great - oh and [s]r=alecf if you need a [super] reviewer to check in against..
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!

since this was at one time +'ed I'm going to take the liberty of changing the 
status back to rtm+
Whiteboard: [rtm need info] fix in hand. → [rtm+] fix in hand.
Agreed.  This should be "rtm+" again.

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.
*** Bug 55176 has been marked as a duplicate of this bug. ***
the difference is that "na" is now a string...sounds good sr=alecf
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"?
"nothing available" gives the user feedback. It lets them know that the URL bar
is not simply broken.
"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.

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

PDT says patch looks like too much risk for too little benefit this late in the
cycle. RTM-
Whiteboard: [rtm+] fix in hand. → [rtm-] fix in hand.
ops, I confused the bugs. Sorry. For the drop-down, this makes sense, of course.

Can we get this into the trunk, please?
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].
Whiteboard: [rtm-] fix in hand. → [rtm need info] fix in hand.
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!
Fix already checked in to trunk
Whiteboard: [rtm need info] fix in hand. → [rtm need info] fix in hand. Fix on trunk
this should get a rtm+, since the patch is already on the trunk.

rtm++
Whiteboard: [rtm need info] fix in hand. Fix on trunk → [rtm++] fix in hand. Fix on trunk
I think, the fix for this bug caused critical bug 57246.
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.
I haven't seen any new string added in the fix! They are using existing one.
If this fix goes onto the branch, then we must also land the fix for bug 57246
on the branch.
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.
Whiteboard: [rtm++] fix in hand. Fix on trunk → [rtm need info] fix in hand. Fix on trunk
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.
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.
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?
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/.
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.


And that's the problem, we cannot update the packages again.
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.
*** Bug 57723 has been marked as a duplicate of this bug. ***
Hi, Radha:


Has this bug been reviewed & super-reviewed? Your latest patch seems to remove
the dependency on l10n group.
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.  
> 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>.

sorry, I meant Ben Goodger
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 
yay, good to see this sr=alecf
Marking rtm+ to get on today's PDT radar
Whiteboard: [rtm need info] fix in hand. Fix on trunk → [rtm+] fix in hand. Fix on trunk
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,
Did the Hyatt and AlecF reviews apply to the 10/24/00 15:47 patch?
Whiteboard: [rtm+] fix in hand. Fix on trunk → [rtm need info] fix in hand. Fix on trunk
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.
Whiteboard: [rtm need info] fix in hand. Fix on trunk → [rtm+] fix in hand. Fix on trunk
Whiteboard: [rtm+] fix in hand. Fix on trunk → [rtm+] fix in hand. Fix on trunk [InLimbo-OOH]
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,
We are moving toward the candidate.  Please check this fix into the trunk so we 
can get get some cook time.
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. 
PDT marking [rtm++]. This bug is now out of limbo and approved for checkin to
the branch. Please check in ASAP.
Whiteboard: [rtm+] fix in hand. Fix on trunk [InLimbo-OOH] → [rtm++] fix in hand. Fix on trunk [InLimbo-OOH]
Whiteboard: [rtm++] fix in hand. Fix on trunk [InLimbo-OOH] → [rtm++] fix in hand. Fix on trunk
Fix checked in to the branch too.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Verified fixed on branch with commercial release build 2000-10-31-14. Adding
vtrunk keyword.
Keywords: vtrunk
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.
Status: RESOLVED → VERIFIED
Keywords: vtrunk
Component: History: Session → Document Navigation
QA Contact: claudius → docshell
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: