420354 - History not shown in sidebar when sorted by date or by date and site

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Cédric Corazza]

On a clean profile : browsing to several sites : the list does appear in History menu, but when using Display > Sidebar > History : no sites are shown. Clicking on the Sort button (default is By Date radio button), then choosing By site, sites list appears.
Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.9b4pre) Gecko/2008022904 Minefield/3.0b4pre

Comment 1

[Dietrich Ayala (:dietrich)]

I'm not able to reproduce this on latest nightly. do you see the "Today" folder? If so, is the expected history contained in the contents of that folder?

Comment 2

[Cédric Corazza]

This still occurs on latest fr tinderbox build (Mozilla/5.0 (X11; U; Linux i686; fr; rv:1.9b4pre) Gecko/2008030304 Minefield/3.0b4pre) but not on en-US latest nightly, where the folder "Today" appears.
So, I guess I should requalify this bug to a fr l10n bug?

Comment 3

[Dietrich Ayala (:dietrich)]

(In reply to comment #2)
> So, I guess I should requalify this bug to a fr l10n bug?
> 

sounds like it, yes. thanks for checking.

Comment 4

[Cédric Corazza]

I saw there was no line at the end of file l10n/fr/toolkit/chrome/places/places.properties.
I make a try with adding one.

Comment 5

[Cédric Corazza]

I reproduce this bug on Windows XP (I don't know if it's the same on Mac).
I think I found what is wrong. In the l10n/fr/toolkit/chrome/places/places.properties file, the string finduri-AgeInDays-is-0=Aujourd'hui contains an apostrophe. When I removed it, the bug disappeared; when I tried to escape it with a backslash (\'), the bug came back. I even tried to surround the string with quotes, same bug.
I don't know how to fix this and I guess this is a l12y problem. Could someone help me here?

Comment 6

[Cédric Corazza]

Changing the old summary "History not shown in sidebar with clean profile on first launch" to make it more accurate

Comment 7

[Benoit]

I'm not sure it is relevant, but it seems that our string (finduri-AgeInDays-is-0) is passed unescaped to an SQL query here:
http://mxr.mozilla.org/mozilla/source/toolkit/components/places/src/nsNavHistory.cpp#2993

Since "Aujourd'hui" contains a quote character ('), I think this would cause a parse error around the dayTitle field when executing the SQL query.

Comment 8

[Ondrej Brablc]

(In reply to comment #7)
> I'm not sure it is relevant, but it seems that our string
> (finduri-AgeInDays-is-0) is passed unescaped to an SQL query here:
> http://mxr.mozilla.org/mozilla/source/toolkit/components/places/src/nsNavHistory.cpp#2993
> 
> Since "Aujourd'hui" contains a quote character ('), I think this would cause a
> parse error around the dayTitle field when executing the SQL query.
> 

OOPS. Thanks for analysis. I will fix it.

Comment 9

[Shawn Wilsher :sdwilsh]

Why are we dynamically building a query string anyway?  The reason for the bind parameter api's exactly because of things like this...

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Shawn, can you file a follow up on the issue from comment #9?

In the meantime, this goes to l10n::fr ...

Comment 11

[Ondrej Brablc]

(In reply to comment #10)
> Shawn, can you file a follow up on the issue from comment #9?
> 
> In the meantime, this goes to l10n::fr ...
> 

I'm afraid it is really my problem in Places. Localizers cannot do anything better and it could be related to multiple languages. I will try to solve it with binding.

Comment 12

[Shawn Wilsher :sdwilsh]

(In reply to comment #10)
> Shawn, can you file a follow up on the issue from comment #9?
Filed bug 421115

Comment 13

[:Gavin Sharp [email: [e-mail]]]

(In reply to comment #10)
> In the meantime, this goes to l10n::fr ...

This is a Places bug...

Comment 14

[Ondrej Brablc]

Attachment: Refactor and enhance parameter binding

This patch refactors the way parameters are bound in history service. It replaces position based parameters with named parameters. This makes the code more robust and is much more readable. For instance SelectAsSite query:

-               "&beginTime='||?1||'&endTime='||?2, "
+               "&beginTime='||:begin_time||'&endTime='||:end_time, "

Previously it was not easy to find what does ?1 refer to, now it is obvious.

Thanks to this it is now possible to add additional named parameters in PlacesSQLQueryBuilder. So the day names can now be bound rather then injected to the SQL string. French and all other locals should work fine now.

Comment 15

[Ondrej Brablc]

Comment on attachment 307736 [details] [diff] [review]
Refactor and enhance parameter binding

I found one more candidate for binding.

Comment 16

[Ondrej Brablc]

Attachment: Refactor and enhance parameter binding v2

Changes since first patcy:
- binding added for (local) 
- fixed incorrect sql for uriIsPrefix (tested using parts of bug 384226)

Comment 18

[Dietrich Ayala (:dietrich)]

Comment on attachment 308400 [details] [diff] [review]
 Refactor and enhance parameter binding v2

nice win on readability and compactness in the parameter bindings, as well as removing the fragility of the parameter order requirement (albeit trading off for more complexity in the helpers).

can you please confirm that we have a test that exercises the multiple query functionality? if not, please add one. also, please add a test specific to the problem this bug addresses.

r=me otherwise.

>   nsresult ConstructQueryString(const nsCOMArray<nsNavHistoryQuery>& aQueries, 
>                                 nsNavHistoryQueryOptions *aOptions,
>                                 nsCString& queryString,
>-                                PRBool& aParamsPresent);
>+                                PRBool& aParamsPresent,
>+                                StringHash & aAddParams);

nit: remove space

Comment 19

[Ondrej Brablc]

Attachment: for check-in

I have modified places.properties locally to include single quote and saw that it failed without this patch. With patch applied it was shown correctly. I'm not sure if I can create a test that would address this bug. It is covered with test_history_sidebar.js and would be visible (the query would fail), if the tests were executed with fr-FR locale.

I would prefer if multiple queries and complex queries get tested with bug 384226. I can take that bug over, but it is not blocking.

Comment 20

[Ondrej Brablc]

Attachment: Refactored parameter binding + multi query test + fix for it

I have added a simple test for multiple queries - and it did not PASS. Current code does this:
  WHERE original AND additional1 OR additional2
What is interpreted as:
  WHERE (original AND additional1) OR additional2
It must be changed to:
  WHERE original AND (additional1 OR additional2)

It is actually another bug, but I hope we can fix it with this patch.

Comment 21

[Dietrich Ayala (:dietrich)]

Comment on attachment 310516 [details] [diff] [review]
Refactored parameter binding + multi query test + fix for it

the tests win yet again ;)

r=me.

Comment 22

[Dietrich Ayala (:dietrich)]

Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.277; previous revision: 1.276
done
Checking in toolkit/components/places/src/nsNavHistory.h;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v  <--  nsNavHistory.h
new revision: 1.150; previous revision: 1.149
done
RCS file: /cvsroot/mozilla/toolkit/components/places/tests/unit/test_multi_queries.js,v
done
Checking in toolkit/components/places/tests/unit/test_multi_queries.js;
/cvsroot/mozilla/toolkit/components/places/tests/unit/test_multi_queries.js,v  <--  test_multi_queries.js
initial revision: 1.1
done

Comment 23

[Tracy Walker [:tracy]]

verified with: 
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5pre)
Gecko/2008032106 Minefield/3.0b5pre
and
Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.4; en-US; rv:1.9b5pre)
Gecko/2008032104 Minefield/3.0b5pre

Comment 24

[Tuukka Tolvanen (sp3000)]

In member function ‘PRUint32 IndexGetter::For(const char*)’:
http://mxr.mozilla.org/mozilla/source/toolkit/components/places/src/nsNavHistory.cpp#4837
warning: negative integer implicitly converted to unsigned type

Comment 25

[Tuukka Tolvanen (sp3000)]

In member function ‘nsresult nsNavHistory::ConstructQueryString(const nsCOMArray<nsNavHistoryQuery>&, nsNavHistoryQueryOptions*, nsCString&, PRBool&, nsDataHashtable<nsCStringHashKey, nsCString>&)’:
http://mxr.mozilla.org/mozilla/source/toolkit/components/places/src/nsNavHistory.cpp#3298
warning: unused variable ‘clauseParameters’

Comment 26

[Ondrej Brablc]

I should have filed a new bug instead of reopening, sorry for that. See bug 424577.

Comment 27

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv